Re: [Qemu-block] [PATCH 05/14] nbd/client: Drop pointless buf variable

[Vladimir Sementsov-Ogievskiy]

05.12.2018 19:29, Eric Blake wrote:
> On 12/5/18 9:59 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 01.12.2018 1:03, Eric Blake wrote:
>>> There's no need to read into a temporary buffer (oversized
>>> since commit 7d3123e1) followed by a byteswap into a uint64_t
>>> to check for a magic number via memcmp(), when the code
>>> immediately below demonstrates reading into the uint64_t then
>>> byteswapping in place and checking for a magic number via
>>> integer math.  What's more, having a different error message
>>> when the server's first reply byte is 0 is unusual - it's no
>>> different from any other wrong magic number, and we already
>>> detected short reads.
>>>
>>> Signed-off-by: Eric Blake <address@hidden>
>>> ---
>>>    nbd/nbd-internal.h |  1 +
>>>    nbd/client.c       | 14 +++-----------
>>>    2 files changed, 4 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h
>>> index eeff78d3c98..306a533dcd1 100644
>>> --- a/nbd/nbd-internal.h
>>> +++ b/nbd/nbd-internal.h
>>> @@ -46,6 +46,7 @@
>>>    /* Size of oldstyle negotiation */
>>>    #define NBD_OLDSTYLE_NEGOTIATE_SIZE (8 + 8 + 8 + 4 + 124)
>>>
>>> +#define NBD_INIT_MAGIC              0x4e42444d41474943LL
>>
>> Worth add comment /* "NBDMAGIC" */
> 
> Maybe.  But if so,
> 
>>
>>>    #define NBD_REQUEST_MAGIC           0x25609513
>>>    #define NBD_OPTS_MAGIC              0x49484156454F5054LL
> 
> this should also get a comment "IHAVEOPT".
> 
>>>    #define NBD_CLIENT_MAGIC            0x0000420281861253LL
>>> diff --git a/nbd/client.c b/nbd/client.c
>>> index 0be89f9e641..17ee24492a4 100644
>>> --- a/nbd/client.c
>>> +++ b/nbd/client.c
>>> @@ -731,7 +731,6 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char 
>>> *name,
>>>                              QIOChannel **outioc, NBDExportInfo *info,
>>>                              Error **errp)
>>>    {
>>> -    char buf[256];
>>>        uint64_t magic;
>>>        int rc;
>>>        bool zeroes = true;
>>> @@ -752,21 +751,14 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char 
>>> *name,
>>>            goto fail;
>>>        }
>>>
>>> -    if (nbd_read(ioc, buf, 8, errp) < 0) {
>>> +    if (nbd_read(ioc, &magic, sizeof(magic), errp) < 0) {
>>>            error_prepend(errp, "Failed to read data: ");
>>
>> may be, change to "Failed to read magic: ", as in code below
> 
> Argument for: consistency sake. Argument against: having distinct messages 
> lets you debug which of the two magic strings was wrong.  I'm not sure I have 
> a strong preference.
> 
>>
>>>            goto fail;
>>>        }
>>> -
>>> -    buf[8] = '\0';
>>> -    if (strlen(buf) == 0) {
>>> -        error_setg(errp, "Server connection closed unexpectedly");
>>> -        goto fail;
>>> -    }
>>> -
>>> -    magic = ldq_be_p(buf);
>>> +    magic = be64_to_cpu(magic);
>>
>> Isn't it better to use be64_to_cpus?
> 
> No. We're intentionally getting rid of that because of clang; see commit 
> 80c7c2b0.
> 
> 

Ok, thanks. In this case it should be safe, but if we decided to avoid these 
functions in general than OK.
Hmm, not in general, but only in nbd.. Strange, why not in qcow2 for ex?, 
anyway, with or without other tiny things:

Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>



-- 
Best regards,
Vladimir