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 <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
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.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
