Re: [Qemu-devel] [PATCH 2/2] nbd/server: add assert to nbd_negotiate_handle_info

[Vladimir Sementsov-Ogievskiy]

02.11.2017 21:06, Eric Blake wrote:
> On 11/01/2017 10:42 AM, Vladimir Sementsov-Ogievskiy wrote:
> > Add an assert here to make last length assignment meaningful and
> > following return without tail dropping obvious.
> Not quite sure I followed your explanation, which means it's difficult
> for me to propose an alternative wording.  Maybe:
>
> Add an assert here to make it obvious that the prior loop consumed the
> rest of the input, and that all further code in the function is focused
> on output.
>
> On the other hand, if you are okay with it, I wouldn't mind squashing
> the first and second patches into one (as the first patch is then easier
> to read when it is obvious that we used the wrong length variable).  But
> until I get your feedback (on either squashing the two patches or
> tweaking the wording), I'm just placing your patches as-is on my NBD
> queue for inclusion prior to rc0.


in commit message I mean two things:
1. assignment to length in the previous loop is useless without an assert
2. firstly I thought that the following return statement is a bug as it 
doesn't
drop payload tail. And only then I noticed previous "if (requests != 
length / sizeof(request))".
With an assert the correctness of the code is more evident.

However you may reword or squash it as you want, all is good.

> > Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> > ---
> >   nbd/server.c | 1 +
> >   1 file changed, 1 insertion(+)
> Reviewed-by: Eric Blake <address@hidden>


--
Best regards,
Vladimir