|
From: | Vladimir Sementsov-Ogievskiy |
Subject: | Re: [Qemu-devel] [PATCH 2/2] nbd/server: add assert to nbd_negotiate_handle_info |
Date: | Fri, 3 Nov 2017 08:50:49 +0300 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 |
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 assert2. 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
[Prev in Thread] | Current Thread | [Next in Thread] |