| Message ID | 20181130220344.3350618-6-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nbd: add qemu-nbd --list | expand |
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> > --- > 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 <rjones@redhat.com> Rich.
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. >
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 <eblake@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 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; > } >
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 <eblake@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 > > 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.
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 <eblake@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 >> >> 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 <vsementsov@virtuozzo.com>
On 12/5/18 10:38 AM, Vladimir Sementsov-Ogievskiy wrote: >>>> - 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?, Peter is working on that - it's a slow process, because he's sending separate patch series per maintainer, so they are not all getting checked in at the same time. But the idea is that once everything is converted, we nuke the *_to_*s variants as unused, and in the meantime, we don't add more uses of it.
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; }
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> --- nbd/nbd-internal.h | 1 + nbd/client.c | 14 +++----------- 2 files changed, 4 insertions(+), 11 deletions(-)