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

[Eric Blake <eblake@redhat.com>]

On 11/30/18 4:30 PM, Richard W.M. Jones wrote:
> On Fri, Nov 30, 2018 at 04:03:34PM -0600, 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 <eblake@redhat.com>
>> ---

>> -
>> -    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);
>>       trace_nbd_receive_negotiate_magic(magic);
>>
>> -    if (memcmp(buf, "NBDMAGIC", 8) != 0) {
>> +    if (magic != NBD_INIT_MAGIC) {
>>           error_setg(errp, "Invalid magic received");
>>           goto fail;
>>       }
> 
> The original code is really odd.  What's the whole strlen about?

Looks like it was added in commit 1d45f8b4 in 2010 in "nbd: Introduce 
NBD named exports." but with no good explanation why it was added in the 
context of the larger patch. Leftover debugging code that should have 
been nuked years ago?

> Anyway this is an obvious improvement so:
> 
> Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
> 
> Rich.
> 

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