From 757a0d05ea22d63e667db08a80b6ec6f1d53a12f Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Fri, 2 Nov 2018 18:11:51 +0300 Subject: nbd: publish _lookup functions These functions are used for formatting pretty trace points. We are going to add some in block/nbd-client, so, let's publish all these functions at once. Note, that nbd_reply_type_lookup is already published, and constants, "named" by these functions live in include/block/nbd.h too. Signed-off-by: Vladimir Sementsov-Ogievskiy Message-Id: <20181102151152.288399-3-vsementsov@virtuozzo.com> Reviewed-by: Eric Blake Signed-off-by: Eric Blake --- include/block/nbd.h | 5 +++++ nbd/nbd-internal.h | 5 ----- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/include/block/nbd.h b/include/block/nbd.h index 6a5bfe5d55..65402d3396 100644 --- a/include/block/nbd.h +++ b/include/block/nbd.h @@ -343,5 +343,10 @@ static inline bool nbd_reply_is_structured(NBDReply *reply) } const char *nbd_reply_type_lookup(uint16_t type); +const char *nbd_opt_lookup(uint32_t opt); +const char *nbd_rep_lookup(uint32_t rep); +const char *nbd_info_lookup(uint16_t info); +const char *nbd_cmd_lookup(uint16_t info); +const char *nbd_err_lookup(int err); #endif diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h index eeff78d3c9..f38be9ebaa 100644 --- a/nbd/nbd-internal.h +++ b/nbd/nbd-internal.h @@ -100,11 +100,6 @@ struct NBDTLSHandshakeData { void nbd_tls_handshake(QIOTask *task, void *opaque); -const char *nbd_opt_lookup(uint32_t opt); -const char *nbd_rep_lookup(uint32_t rep); -const char *nbd_info_lookup(uint16_t info); -const char *nbd_cmd_lookup(uint16_t info); -const char *nbd_err_lookup(int err); int nbd_drop(QIOChannel *ioc, size_t size, Error **errp); -- cgit v1.2.3 From bee21ef0950c8b109d3bad05a3c3f2d94ec1a3af Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Tue, 18 Dec 2018 16:57:13 -0600 Subject: nbd/client: Trace all server option error messages Not all servers send free-form text alongside option error replies, but for servers that do (such as qemu), we pass the server's message as a hint alongside our own error reporting. However, it would also be useful to trace such server messages, since we can't guarantee how the hint may be consumed. Signed-off-by: Eric Blake Message-Id: <20181218225714.284495-3-eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy --- nbd/client.c | 2 ++ nbd/trace-events | 1 + 2 files changed, 3 insertions(+) diff --git a/nbd/client.c b/nbd/client.c index b4d457a19a..0ad7147ed9 100644 --- a/nbd/client.c +++ b/nbd/client.c @@ -171,6 +171,8 @@ static int nbd_handle_reply_err(QIOChannel *ioc, NBDOptionReply *reply, goto cleanup; } msg[reply->length] = '\0'; + trace_nbd_server_error_msg(reply->type, + nbd_reply_type_lookup(reply->type), msg); } switch (reply->type) { diff --git a/nbd/trace-events b/nbd/trace-events index 5e1d4afe8e..5492042acb 100644 --- a/nbd/trace-events +++ b/nbd/trace-events @@ -1,6 +1,7 @@ # nbd/client.c nbd_send_option_request(uint32_t opt, const char *name, uint32_t len) "Sending option request %" PRIu32" (%s), len %" PRIu32 nbd_receive_option_reply(uint32_t option, const char *optname, uint32_t type, const char *typename, uint32_t length) "Received option reply %" PRIu32" (%s), type %" PRIu32" (%s), len %" PRIu32 +nbd_server_error_msg(uint32_t err, const char *type, const char *msg) "server reported error 0x%" PRIx32 " (%s) with additional message: %s" nbd_reply_err_unsup(uint32_t option, const char *name) "server doesn't understand request %" PRIu32 " (%s), attempting fallback" nbd_opt_go_start(const char *name) "Attempting NBD_OPT_GO for export '%s'" nbd_opt_go_success(void) "Export is good to go" -- cgit v1.2.3 From d8b4bad846f08ff0f167b46dc156a5310b750484 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Fri, 2 Nov 2018 18:11:52 +0300 Subject: block/nbd-client: use traces instead of noisy error_report_err Reduce extra noise of nbd-client, change 083 correspondingly. In various commits (be41c100 in 2.10, f140e300 in 2.11, 78a33ab in 2.12), we added spots where qemu as an NBD client would report problems communicating with the server to stderr, because there was no where else to send the error to. However, this is racy, particularly since the most common source of these errors is when either the client or the server abruptly hangs up, leaving one coroutine to report the error only if it wins (or loses) the race in attempting the read from the server before another thread completes its cleanup of a protocol error that caused the disconnect in the first place. The race is also apparent in the fact that differences in the flush behavior of the server can alter the frequency of encountering the race in the client (see commit 6d39db96). Rather than polluting stderr, it's better to just trace these situations, for use by developers debugging a flaky connection, particularly since the real error that either triggers the abrupt disconnection in the first place, or that results from the EIO when a request can't receive a reply, DOES make it back to the user in the normal Error propagation channels. Signed-off-by: Vladimir Sementsov-Ogievskiy Message-Id: <20181102151152.288399-4-vsementsov@virtuozzo.com> [eblake: drop depedence on error hint, enhance commit message] Signed-off-by: Eric Blake --- block/nbd-client.c | 23 +++++++++++++++++++---- block/trace-events | 4 ++++ tests/qemu-iotests/083.out | 28 ---------------------------- 3 files changed, 23 insertions(+), 32 deletions(-) diff --git a/block/nbd-client.c b/block/nbd-client.c index fc5b7eda8e..ef32075971 100644 --- a/block/nbd-client.c +++ b/block/nbd-client.c @@ -28,6 +28,8 @@ */ #include "qemu/osdep.h" + +#include "trace.h" #include "qapi/error.h" #include "nbd-client.h" @@ -79,7 +81,8 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque) assert(s->reply.handle == 0); ret = nbd_receive_reply(s->ioc, &s->reply, &local_err); if (local_err) { - error_report_err(local_err); + trace_nbd_read_reply_entry_fail(ret, error_get_pretty(local_err)); + error_free(local_err); } if (ret <= 0) { break; @@ -771,7 +774,11 @@ static int nbd_co_request(BlockDriverState *bs, NBDRequest *request, ret = nbd_co_receive_return_code(client, request->handle, &local_err); if (local_err) { - error_report_err(local_err); + trace_nbd_co_request_fail(request->from, request->len, request->handle, + request->flags, request->type, + nbd_cmd_lookup(request->type), + ret, error_get_pretty(local_err)); + error_free(local_err); } return ret; } @@ -802,7 +809,11 @@ int nbd_client_co_preadv(BlockDriverState *bs, uint64_t offset, ret = nbd_co_receive_cmdread_reply(client, request.handle, offset, qiov, &local_err); if (local_err) { - error_report_err(local_err); + trace_nbd_co_request_fail(request.from, request.len, request.handle, + request.flags, request.type, + nbd_cmd_lookup(request.type), + ret, error_get_pretty(local_err)); + error_free(local_err); } return ret; } @@ -925,7 +936,11 @@ int coroutine_fn nbd_client_co_block_status(BlockDriverState *bs, ret = nbd_co_receive_blockstatus_reply(client, request.handle, bytes, &extent, &local_err); if (local_err) { - error_report_err(local_err); + trace_nbd_co_request_fail(request.from, request.len, request.handle, + request.flags, request.type, + nbd_cmd_lookup(request.type), + ret, error_get_pretty(local_err)); + error_free(local_err); } if (ret < 0) { return ret; diff --git a/block/trace-events b/block/trace-events index 3e8c47bb24..693c14c443 100644 --- a/block/trace-events +++ b/block/trace-events @@ -156,3 +156,7 @@ nvme_cmd_map_qiov_iov(void *s, int i, void *page, int pages) "s %p iov[%d] %p pa # block/iscsi.c iscsi_xcopy(void *src_lun, uint64_t src_off, void *dst_lun, uint64_t dst_off, uint64_t bytes, int ret) "src_lun %p offset %"PRIu64" dst_lun %p offset %"PRIu64" bytes %"PRIu64" ret %d" + +# block/nbd-client.c +nbd_read_reply_entry_fail(int ret, const char *err) "ret = %d, err: %s" +nbd_co_request_fail(uint64_t from, uint32_t len, uint64_t handle, uint16_t flags, uint16_t type, const char *name, int ret, const char *err) "Request failed { .from = %" PRIu64", .len = %" PRIu32 ", .handle = %" PRIu64 ", .flags = 0x%" PRIx16 ", .type = %" PRIu16 " (%s) } ret = %d, err: %s" diff --git a/tests/qemu-iotests/083.out b/tests/qemu-iotests/083.out index f9af8bb691..7419722cd7 100644 --- a/tests/qemu-iotests/083.out +++ b/tests/qemu-iotests/083.out @@ -41,8 +41,6 @@ can't open device nbd+tcp://127.0.0.1:PORT/foo === Check disconnect after neg2 === -Unable to read from socket: Connection reset by peer -Connection closed read failed: Input/output error === Check disconnect 8 neg2 === @@ -55,40 +53,30 @@ can't open device nbd+tcp://127.0.0.1:PORT/foo === Check disconnect before request === -Unable to read from socket: Connection reset by peer -Connection closed read failed: Input/output error === Check disconnect after request === -Connection closed read failed: Input/output error === Check disconnect before reply === -Connection closed read failed: Input/output error === Check disconnect after reply === -Unexpected end-of-file before all bytes were read read failed: Input/output error === Check disconnect 4 reply === -Unexpected end-of-file before all bytes were read -Connection closed read failed: Input/output error === Check disconnect 8 reply === -Unexpected end-of-file before all bytes were read -Connection closed read failed: Input/output error === Check disconnect before data === -Unexpected end-of-file before all bytes were read read failed: Input/output error === Check disconnect after data === @@ -118,8 +106,6 @@ can't open device nbd+tcp://127.0.0.1:PORT/ === Check disconnect after neg-classic === -Unable to read from socket: Connection reset by peer -Connection closed read failed: Input/output error === Check disconnect before neg1 === @@ -164,8 +150,6 @@ can't open device nbd+unix:///foo?socket=TEST_DIR/nbd.sock === Check disconnect after neg2 === -Unable to read from socket: Connection reset by peer -Connection closed read failed: Input/output error === Check disconnect 8 neg2 === @@ -178,40 +162,30 @@ can't open device nbd+unix:///foo?socket=TEST_DIR/nbd.sock === Check disconnect before request === -Unable to read from socket: Connection reset by peer -Connection closed read failed: Input/output error === Check disconnect after request === -Connection closed read failed: Input/output error === Check disconnect before reply === -Connection closed read failed: Input/output error === Check disconnect after reply === -Unexpected end-of-file before all bytes were read read failed: Input/output error === Check disconnect 4 reply === -Unexpected end-of-file before all bytes were read -Connection closed read failed: Input/output error === Check disconnect 8 reply === -Unexpected end-of-file before all bytes were read -Connection closed read failed: Input/output error === Check disconnect before data === -Unexpected end-of-file before all bytes were read read failed: Input/output error === Check disconnect after data === @@ -241,8 +215,6 @@ can't open device nbd+unix:///?socket=TEST_DIR/nbd.sock === Check disconnect after neg-classic === -Unable to read from socket: Connection reset by peer -Connection closed read failed: Input/output error *** done -- cgit v1.2.3 From 3ba1b7baf4b2518e8efdbe50175d909b79c21596 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Sat, 15 Dec 2018 07:53:03 -0600 Subject: qemu-nbd: Use program name in error messages This changes output from: $ qemu-nbd nosuch Failed to blk_new_open 'nosuch': Could not open 'nosuch': No such file or directory to something more consistent with qemu-img and qemu: $ qemu-nbd nosuch qemu-nbd: Failed to blk_new_open 'nosuch': Could not open 'nosuch': No such file or directory Update the lone affected test to match. (Hmm - is it sad that we don't do much testing of expected failures?) Signed-off-by: Eric Blake Reviewed-by: Richard W.M. Jones Reviewed-by: Vladimir Sementsov-Ogievskiy Message-Id: <20181215135324.152629-2-eblake@redhat.com> --- qemu-nbd.c | 1 + tests/qemu-iotests/233.out | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/qemu-nbd.c b/qemu-nbd.c index ca7109652e..e169b839ec 100644 --- a/qemu-nbd.c +++ b/qemu-nbd.c @@ -571,6 +571,7 @@ int main(int argc, char **argv) #endif module_call_init(MODULE_INIT_TRACE); + error_set_progname(argv[0]); qcrypto_init(&error_fatal); module_call_init(MODULE_INIT_QOM); diff --git a/tests/qemu-iotests/233.out b/tests/qemu-iotests/233.out index 94acd9b947..5f416721b0 100644 --- a/tests/qemu-iotests/233.out +++ b/tests/qemu-iotests/233.out @@ -27,7 +27,7 @@ virtual size: 64M (67108864 bytes) disk size: unavailable == check TLS with different CA fails == -option negotiation failed: Verify failed: No certificate was found. +qemu-nbd: option negotiation failed: Verify failed: No certificate was found. qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=PORT,tls-creds=tls0': The certificate hasn't got a known issuer == perform I/O over TLS == -- cgit v1.2.3 From ba2d3b3ab217784822e4232f0acd71fc523d571f Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Sat, 15 Dec 2018 07:53:04 -0600 Subject: nbd: Document timeline of various features It can be useful to figure out which NBD protocol features are exposed by a server, as well as what features a client will take advantage of if available, for a given qemu release. It's not always precise to base features on version numbers (thanks to downstream backports), but any documentation is better than making users search through git logs themselves. This patch originally stemmed from a request to document that pristine 3.0 has a known bug where NBD_OPT_LIST_META_CONTEXT with 0 queries forgot to advertise an available "qemu:dirty-bitmap" context, but documenting bugs like this (or the fact that 3.0 also botched NBD_CMD_CACHE) gets to be too much details, especially since buggy releases will be less likely connection targets over time. Instead, I chose to just remind users to check stable release branches. Suggested-by: Vladimir Sementsov-Ogievskiy Signed-off-by: Eric Blake Message-Id: <20181215135324.152629-3-eblake@redhat.com> Reviewed-by: Richard W.M. Jones Reviewed-by: Vladimir Sementsov-Ogievskiy --- docs/interop/nbd.txt | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/docs/interop/nbd.txt b/docs/interop/nbd.txt index 77b5f45911..fc64473e02 100644 --- a/docs/interop/nbd.txt +++ b/docs/interop/nbd.txt @@ -15,7 +15,6 @@ Qemu supports the "base:allocation" metadata context as defined in the NBD protocol specification, and also defines an additional metadata namespace "qemu". - == "qemu" namespace == The "qemu" namespace currently contains only one type of context, @@ -36,3 +35,21 @@ in addition to "qemu:dirty-bitmap:": namespace. * "qemu:dirty-bitmap:" - returns list of all available dirty-bitmap metadata contexts. + += Features by version = + +The following list documents which qemu version first implemented +various features (both as a server exposing the feature, and as a +client taking advantage of the feature when present), to make it +easier to plan for cross-version interoperability. Note that in +several cases, the initial release containing a feature may require +additional patches from the corresponding stable branch to fix bugs in +the operation of that feature. + +* 2.6: NBD_OPT_STARTTLS with TLS X.509 Certificates +* 2.8: NBD_CMD_WRITE_ZEROES +* 2.10: NBD_OPT_GO, NBD_INFO_BLOCK +* 2.11: NBD_OPT_STRUCTURED_REPLY +* 2.12: NBD_CMD_BLOCK_STATUS for "base:allocation" +* 3.0: NBD_OPT_STARTTLS with TLS Pre-Shared Keys (PSK), +NBD_CMD_BLOCK_STATUS for "qemu:dirty-bitmap:", NBD_CMD_CACHE -- cgit v1.2.3 From 6c5c035138218a384a229e7b6b9cf51451621c6a Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Sat, 15 Dec 2018 07:53:07 -0600 Subject: nbd/client: More consistent error messages Consolidate on using decimal (not hex), on outputting the option reply name (not just value), and a consistent comma between clauses, when the client reports protocol discrepancies from the server. While it won't affect normal operation, it makes debugging additions easier. Signed-off-by: Eric Blake Reviewed-by: Richard W.M. Jones Reviewed-by: Vladimir Sementsov-Ogievskiy Message-Id: <20181215135324.152629-6-eblake@redhat.com> --- nbd/client.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/nbd/client.c b/nbd/client.c index 0ad7147ed9..e77414711b 100644 --- a/nbd/client.c +++ b/nbd/client.c @@ -132,8 +132,9 @@ static int nbd_receive_option_reply(QIOChannel *ioc, uint32_t opt, return -1; } if (reply->option != opt) { - error_setg(errp, "Unexpected option type %x expected %x", - reply->option, opt); + error_setg(errp, "Unexpected option type %u (%s), expected %u (%s)", + reply->option, nbd_opt_lookup(reply->option), + opt, nbd_opt_lookup(opt)); nbd_send_opt_abort(ioc); return -1; } @@ -267,8 +268,9 @@ static int nbd_receive_list(QIOChannel *ioc, const char *want, bool *match, } return 0; } else if (reply.type != NBD_REP_SERVER) { - error_setg(errp, "Unexpected reply type %" PRIx32 " expected %x", - reply.type, NBD_REP_SERVER); + error_setg(errp, "Unexpected reply type %u (%s), expected %u (%s)", + reply.type, nbd_rep_lookup(reply.type), + NBD_REP_SERVER, nbd_rep_lookup(NBD_REP_SERVER)); nbd_send_opt_abort(ioc); return -1; } @@ -380,9 +382,9 @@ static int nbd_opt_go(QIOChannel *ioc, const char *wantname, return 1; } if (reply.type != NBD_REP_INFO) { - error_setg(errp, "unexpected reply type %" PRIu32 - " (%s), expected %u", - reply.type, nbd_rep_lookup(reply.type), NBD_REP_INFO); + error_setg(errp, "unexpected reply type %u (%s), expected %u (%s)", + reply.type, nbd_rep_lookup(reply.type), + NBD_REP_INFO, nbd_rep_lookup(NBD_REP_INFO)); nbd_send_opt_abort(ioc); return -1; } @@ -706,8 +708,9 @@ static int nbd_negotiate_simple_meta_context(QIOChannel *ioc, } if (reply.type != NBD_REP_ACK) { - error_setg(errp, "Unexpected reply type %" PRIx32 " expected %x", - reply.type, NBD_REP_ACK); + error_setg(errp, "Unexpected reply type %u (%s), expected %u (%s)", + reply.type, nbd_rep_lookup(reply.type), + NBD_REP_ACK, nbd_rep_lookup(NBD_REP_ACK)); nbd_send_opt_abort(ioc); return -1; } -- cgit v1.2.3 From 3c1fa35d74aabe9c3ab642d2591b087e53d7a616 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Sat, 15 Dec 2018 07:53:08 -0600 Subject: qemu-nbd: Fail earlier for -c/-d on non-linux Connecting to a /dev/nbdN device is a Linux-specific action. We were already masking -c and -d from 'qemu-nbd --help' on non-linux. However, while -d fails with a sensible error message, it took hunting through a couple of files to prove that. What's more, the code for -c doesn't fail until after it has created a pthread and tried to open a device - possibly even printing an error message with %m on a non-Linux platform in spite of the comment that %m is glibc-specific. Make the failure happen sooner, then get rid of stubs that are no longer needed because of the early exits. While at it: tweak the blank newlines in --help output to be consistent, whether or not built on Linux. Signed-off-by: Eric Blake Message-Id: <20181215135324.152629-7-eblake@redhat.com> Reviewed-by: Richard W.M. Jones Reviewed-by: Vladimir Sementsov-Ogievskiy --- nbd/client.c | 18 +----------------- qemu-nbd.c | 21 +++++++++++++++++++-- 2 files changed, 20 insertions(+), 19 deletions(-) diff --git a/nbd/client.c b/nbd/client.c index e77414711b..5a03a84418 100644 --- a/nbd/client.c +++ b/nbd/client.c @@ -1031,23 +1031,7 @@ int nbd_disconnect(int fd) return 0; } -#else -int nbd_init(int fd, QIOChannelSocket *ioc, NBDExportInfo *info, - Error **errp) -{ - error_setg(errp, "nbd_init is only supported on Linux"); - return -ENOTSUP; -} - -int nbd_client(int fd) -{ - return -ENOTSUP; -} -int nbd_disconnect(int fd) -{ - return -ENOTSUP; -} -#endif +#endif /* __linux__ */ int nbd_send_request(QIOChannel *ioc, NBDRequest *request) { diff --git a/qemu-nbd.c b/qemu-nbd.c index e169b839ec..2807e13239 100644 --- a/qemu-nbd.c +++ b/qemu-nbd.c @@ -43,6 +43,12 @@ #include "trace/control.h" #include "qemu-version.h" +#ifdef __linux__ +#define HAVE_NBD_DEVICE 1 +#else +#define HAVE_NBD_DEVICE 0 +#endif + #define SOCKET_PATH "/var/lock/qemu-nbd-%s" #define QEMU_NBD_OPT_CACHE 256 #define QEMU_NBD_OPT_AIO 257 @@ -98,11 +104,11 @@ static void usage(const char *name) " specify tracing options\n" " --fork fork off the server process and exit the parent\n" " once the server is running\n" -#ifdef __linux__ +#if HAVE_NBD_DEVICE +"\n" "Kernel NBD client support:\n" " -c, --connect=DEV connect FILE to the local NBD device DEV\n" " -d, --disconnect disconnect the specified device\n" -"\n" #endif "\n" "Block device options:\n" @@ -236,6 +242,7 @@ static void termsig_handler(int signum) } +#if HAVE_NBD_DEVICE static void *show_parts(void *arg) { char *device = arg; @@ -321,6 +328,7 @@ out: kill(getpid(), SIGTERM); return (void *) EXIT_FAILURE; } +#endif /* HAVE_NBD_DEVICE */ static int nbd_can_accept(void) { @@ -814,6 +822,12 @@ int main(int argc, char **argv) } } +#if !HAVE_NBD_DEVICE + if (disconnect || device) { + error_report("Kernel /dev/nbdN support not available"); + exit(EXIT_FAILURE); + } +#else /* HAVE_NBD_DEVICE */ if (disconnect) { int nbdfd = open(argv[optind], O_RDWR); if (nbdfd < 0) { @@ -829,6 +843,7 @@ int main(int argc, char **argv) return 0; } +#endif if ((device && !verbose) || fork_process) { int stderr_fd[2]; @@ -1006,6 +1021,7 @@ int main(int argc, char **argv) nbd_export_set_description(exp, export_description); if (device) { +#if HAVE_NBD_DEVICE int ret; ret = pthread_create(&client_thread, NULL, nbd_client_thread, device); @@ -1013,6 +1029,7 @@ int main(int argc, char **argv) error_report("Failed to create client thread: %s", strerror(ret)); exit(EXIT_FAILURE); } +#endif } else { /* Shut up GCC warnings. */ memset(&client_thread, 0, sizeof(client_thread)); -- cgit v1.2.3 From ef2e35fcc8e14bcc9366df5fdf53f65d679f8dca Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Sat, 15 Dec 2018 07:53:10 -0600 Subject: nbd/client: Drop pointless buf variable 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. That whole strlen() issue has been present and useless since commit 1d45f8b5 in 2010; perhaps it was leftover debugging (since the correct magic number happens to be ASCII)? Make the error messages more consistent and detailed while touching things. Signed-off-by: Eric Blake Reviewed-by: Richard W.M. Jones Reviewed-by: Vladimir Sementsov-Ogievskiy Message-Id: <20181215135324.152629-9-eblake@redhat.com> --- nbd/client.c | 22 +++++++--------------- nbd/nbd-internal.h | 3 ++- 2 files changed, 9 insertions(+), 16 deletions(-) diff --git a/nbd/client.c b/nbd/client.c index 5a03a84418..f625c207c5 100644 --- a/nbd/client.c +++ b/nbd/client.c @@ -733,7 +733,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; @@ -754,27 +753,20 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, goto fail; } - if (nbd_read(ioc, buf, 8, 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"); + if (nbd_read(ioc, &magic, sizeof(magic), errp) < 0) { + error_prepend(errp, "Failed to read initial magic: "); goto fail; } - - magic = ldq_be_p(buf); + magic = be64_to_cpu(magic); trace_nbd_receive_negotiate_magic(magic); - if (memcmp(buf, "NBDMAGIC", 8) != 0) { - error_setg(errp, "Invalid magic received"); + if (magic != NBD_INIT_MAGIC) { + error_setg(errp, "Bad initial magic received: 0x%" PRIx64, magic); goto fail; } if (nbd_read(ioc, &magic, sizeof(magic), errp) < 0) { - error_prepend(errp, "Failed to read magic: "); + error_prepend(errp, "Failed to read server magic: "); goto fail; } magic = be64_to_cpu(magic); @@ -913,7 +905,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, } info->flags = oldflags; } else { - error_setg(errp, "Bad magic received"); + error_setg(errp, "Bad server magic received: 0x%" PRIx64, magic); goto fail; } diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h index f38be9ebaa..82aa221227 100644 --- a/nbd/nbd-internal.h +++ b/nbd/nbd-internal.h @@ -46,8 +46,9 @@ /* Size of oldstyle negotiation */ #define NBD_OLDSTYLE_NEGOTIATE_SIZE (8 + 8 + 8 + 4 + 124) +#define NBD_INIT_MAGIC 0x4e42444d41474943LL /* ASCII "NBDMAGIC" */ #define NBD_REQUEST_MAGIC 0x25609513 -#define NBD_OPTS_MAGIC 0x49484156454F5054LL +#define NBD_OPTS_MAGIC 0x49484156454F5054LL /* ASCII "IHAVEOPT" */ #define NBD_CLIENT_MAGIC 0x0000420281861253LL #define NBD_REP_MAGIC 0x0003e889045565a9LL -- cgit v1.2.3