Re: [Qemu-devel] [PULL 0/4] RDMA patches

[Philippe Mathieu-Daudé]

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 <address@hidden> 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.

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.

Regards,

Phil.