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 <ebl...@redhat.com>
> ---
> 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
> #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: ");
> 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);
> 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?
Anyway this is an obvious improvement so:
Reviewed-by: Richard W.M. Jones <rjo...@redhat.com>
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW
