[Qemu-devel] [PATCH 1/2] nbd/server: fix nbd_negotiate_handle_info

[Qemu-devel] [PATCH 1/2] nbd/server: fix nbd_negotiate_handle_info

[Vladimir Sementsov-Ogievskiy]

namelen should be here, lenght is unrelated.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 nbd/server.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/nbd/server.c b/nbd/server.c
index 70b40ed27e..7fcec0af7e 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -433,7 +433,7 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint32_t length,
 
     /* Don't bother sending NBD_INFO_NAME unless client requested it */
     if (sendname) {
-        rc = nbd_negotiate_send_info(client, opt, NBD_INFO_NAME, length, name,
+        rc = nbd_negotiate_send_info(client, opt, NBD_INFO_NAME, namelen, name,
                                      errp);
         if (rc < 0) {
             return rc;
-- 
2.11.1

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

[Vladimir Sementsov-Ogievskiy]

Add an assert here to make last length assignment meaningful and
following return without tail dropping obvious.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 nbd/server.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/nbd/server.c b/nbd/server.c
index 7fcec0af7e..bcf0cdb47c 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -423,6 +423,7 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint32_t length,
             break;
         }
     }
+    assert(length == 0);
 
     exp = nbd_export_find(name);
     if (!exp) {
-- 
2.11.1

Re: [Qemu-devel] [PATCH 1/2] nbd/server: fix nbd_negotiate_handle_info

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1599 bytes --]

On 11/01/2017 10:42 AM, Vladimir Sementsov-Ogievskiy wrote:

It's best to send a 0/2 cover letter for a series, even when both
patches are small, as that helps automation tools.

> namelen should be here, lenght is unrelated.

s/lenght/length/

Broken in introduction in commit f37708f6; hence adding qemu-stable in cc.

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  nbd/server.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/nbd/server.c b/nbd/server.c
> index 70b40ed27e..7fcec0af7e 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -433,7 +433,7 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint32_t length,
>  
>      /* Don't bother sending NBD_INFO_NAME unless client requested it */
>      if (sendname) {
> -        rc = nbd_negotiate_send_info(client, opt, NBD_INFO_NAME, length, name,
> +        rc = nbd_negotiate_send_info(client, opt, NBD_INFO_NAME, namelen, name,

Interestingly enough, length == 0 at this point, so we would always
report that the export name is '' (aka the default export), without
actually being a protocol violation.  Doesn't hurt qemu as a client,
since we don't ask for NBD_INFO_NAME, but may break other NBD client
implementations, if they then use NBD_OPT_GO on the '' name expecting it
to resolve to the same non-empty name they just queried on NBD_OPT_INFO.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1181 bytes --]

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.

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  nbd/server.c | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH 1/2] nbd/server: fix nbd_negotiate_handle_info

[Vladimir Sementsov-Ogievskiy]

02.11.2017 20:54, Eric Blake wrote:
> On 11/01/2017 10:42 AM, Vladimir Sementsov-Ogievskiy wrote:
>
> It's best to send a 0/2 cover letter for a series, even when both
> patches are small, as that helps automation tools.

ok.

>
>> namelen should be here, lenght is unrelated.
> s/lenght/length/

my eternal problem...

>
> Broken in introduction in commit f37708f6; hence adding qemu-stable in cc.
>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   nbd/server.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
>> diff --git a/nbd/server.c b/nbd/server.c
>> index 70b40ed27e..7fcec0af7e 100644
>> --- a/nbd/server.c
>> +++ b/nbd/server.c
>> @@ -433,7 +433,7 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint32_t length,
>>   
>>       /* Don't bother sending NBD_INFO_NAME unless client requested it */
>>       if (sendname) {
>> -        rc = nbd_negotiate_send_info(client, opt, NBD_INFO_NAME, length, name,
>> +        rc = nbd_negotiate_send_info(client, opt, NBD_INFO_NAME, namelen, name,
> Interestingly enough, length == 0 at this point, so we would always
> report that the export name is '' (aka the default export), without
> actually being a protocol violation.  Doesn't hurt qemu as a client,
> since we don't ask for NBD_INFO_NAME, but may break other NBD client
> implementations, if they then use NBD_OPT_GO on the '' name expecting it
> to resolve to the same non-empty name they just queried on NBD_OPT_INFO.
>


-- 
Best regards,
Vladimir

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 <vsementsov@virtuozzo.com>
>> ---
>>   nbd/server.c | 1 +
>>   1 file changed, 1 insertion(+)
> Reviewed-by: Eric Blake <eblake@redhat.com>
>


-- 
Best regards,
Vladimir