On 08/02/2018 16:01, Philippe Mathieu-Daudé wrote:
> Hi Marcel,
>
> On 02/08/2018 10:38 AM, Marcel Apfelbaum wrote:
>> On 08/02/2018 14:59, Peter Maydell wrote:
>>> On 5 February 2018 at 10:26, Marcel Apfelbaum <mar...@redhat.com> wrote:
>>>> The following changes since commit
>>>> f24ee107a07f093bd7ed475dd48d7ba57ea3d8fe:
>>>
>>> Hi. The technical details of this pullreq are all fine (pgp
>>> key, format, etc), and it passes my build tests. But I gave
>>> this pullreq a bit of a closer inspection than I normally
>>> would, since it's your first, and there are a few things I
>>> thought worth bringing up:
>>
>> Thanks for doing it!
>>
>>>
>>> (1) I notice that some of the new files in this pullreq are licensed
>>> as "GPL, version 2", rather than "version 2 or any later version".
>>> Did you really mean that? Per 'LICENSE', we have a strong preference
>>> for 2-or-later for new code.
>>>
>>
>> No real preference, I will modify the license.
>>
>>> (2) Some new files have no copyright or license comment at the
>>> top of them. Can you fix that, please?
>>>
>>
>> Sure.
>>
>>> (3) Some of the new headers use kernel-internals __u32 etc types.
>>> This isn't portable. ('HACKING' has some suggestions for types you
>>> might want instead.)
>>>
>>
>> We do not "use" the __u32 types, we just copied a kernel file
>> for structures used for communication between the guest driver
>> and the QEMU code. We had a look on how it is done and
>> we use the model that adds macros __u32 -> uint32_t,
>> so the "__types" do not really create such problems.
>>
>>> (4) One of your patches doesn't have any reviewed-by tags.
>>> We don't always manage to review everything, but it is
>>> nicer if we can get review, especially for patches from
>>> new submaintainers.
>>>
>>
>> The patch did receive several questions/comments and all
>> of them were addressed, but indeed no RB tag was given.
>> Since the patch was in the mailing list for over a month
>> and *was* reviewed, I thought is enough.
>> I will ping Eduardo, he had the latest comments for it.
>>
>>
>>> (5) This is an absolutely enormous diffstat for a single commit:
>>> 26 files changed, 5149 insertions(+), 4 deletions(-)
>>>
>>
>> On the github where the project was developed we have thousands of commits,
>> so it can't be used.
>> It was reviewed closely by one reviewer and got a lot
>> of comments from others.
>> That being said, we will try to split it in a few patches
>> and send a new version.
>
> I spent some time to review this but got lost when it became too
> specific (this is not really my area).
> I was hoping some of the VMware folks could review this.
>
The code was reviewed by someone from Mellanox and
we have an RDMA developer from Oracle looking into it to.
For V10 we will have a second RB for the device, it should be enough.
> KVM related stuffs are hard to test, but we have some qtests (migration
> mostly). Adding some tests for this huge code addition would be really
> great.
>
It is in our plans, yes.
We saw the avocado guys are returning to QEMU, I will ask their help in setting
up an avocado test. It should take some time and we are developing the device
over an year offline making it had to maintain/review, so
we will work on the tests in parallel with the device submission.
Thanks,
Marcel
> Regards,
>
> Phil.
>
