qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Vladimir Sementsov-Ogievskiy
Subject: Re: [Qemu-block] [PATCH 05/14] nbd/client: Drop pointless buf variable
Date: Wed, 5 Dec 2018 15:59:33 +0000

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" */

>   #define NBD_REQUEST_MAGIC           0x25609513
>   #define NBD_OPTS_MAGIC              0x49484156454F5054LL
>   #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

>           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?

>       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;
>       }
> 


-- 
Best regards,
Vladimir

reply via email to

[Prev in Thread] Current Thread [Next in Thread]