[Qemu-devel] [PULL 0/8] NBD patches through 2019-01-05

[Qemu-devel] [PULL 0/8] NBD patches through 2019-01-05

[Eric Blake]

The following changes since commit 9b2e891ec5ccdb4a7d583b77988848282606fdea:

  Merge remote-tracking branch 'remotes/marcel/tags/rdma-pull-request' into staging (2018-12-22 11:25:31 +0000)

are available in the Git repository at:

  https://repo.or.cz/qemu/ericb.git tags/pull-nbd-2019-01-05

for you to fetch changes up to ef2e35fcc8e14bcc9366df5fdf53f65d679f8dca:

  nbd/client: Drop pointless buf variable (2019-01-05 07:53:22 -0600)

----------------------------------------------------------------
nbd patches for 2019-01-05

Error and trace improvements in NBD code, such as less noise for
common disconnect scenarios.

- Vladimir Sementsov-Ogievskiy: 0/3 nbd-client: drop extra error noise
- Eric Blake: portions of 0/22 nbd: add qemu-nbd --list

----------------------------------------------------------------
Eric Blake (6):
      nbd/client: Trace all server option error messages
      qemu-nbd: Use program name in error messages
      nbd: Document timeline of various features
      nbd/client: More consistent error messages
      qemu-nbd: Fail earlier for -c/-d on non-linux
      nbd/client: Drop pointless buf variable

Vladimir Sementsov-Ogievskiy (2):
      nbd: publish _lookup functions
      block/nbd-client: use traces instead of noisy error_report_err

 docs/interop/nbd.txt       | 19 +++++++++++++-
 include/block/nbd.h        |  5 ++++
 nbd/nbd-internal.h         |  8 ++----
 block/nbd-client.c         | 23 ++++++++++++++---
 nbd/client.c               | 63 ++++++++++++++++------------------------------
 qemu-nbd.c                 | 22 ++++++++++++++--
 block/trace-events         |  4 +++
 nbd/trace-events           |  1 +
 tests/qemu-iotests/083.out | 28 ---------------------
 tests/qemu-iotests/233.out |  2 +-
 10 files changed, 92 insertions(+), 83 deletions(-)

-- 
2.20.1

[Qemu-devel] [PULL 1/8] nbd: publish _lookup functions

[Eric Blake]

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

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 <vsementsov@virtuozzo.com>
Message-Id: <20181102151152.288399-3-vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 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 6a5bfe5d559..65402d33964 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 eeff78d3c98..f38be9ebaaf 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);

-- 
2.20.1

[Qemu-devel] [PULL 2/8] nbd/client: Trace all server option error messages

[Eric Blake]

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 <eblake@redhat.com>
Message-Id: <20181218225714.284495-3-eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 nbd/client.c     | 2 ++
 nbd/trace-events | 1 +
 2 files changed, 3 insertions(+)

diff --git a/nbd/client.c b/nbd/client.c
index b4d457a19ad..0ad7147ed95 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 5e1d4afe8e6..5492042acbf 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"
-- 
2.20.1

[Qemu-devel] [PULL 3/8] block/nbd-client: use traces instead of noisy error_report_err

[Eric Blake]

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

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 <vsementsov@virtuozzo.com>
Message-Id: <20181102151152.288399-4-vsementsov@virtuozzo.com>
[eblake: drop depedence on error hint, enhance commit message]
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 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 fc5b7eda8ee..ef320759716 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 3e8c47bb243..693c14c4435 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 f9af8bb6918..7419722cd7a 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
-- 
2.20.1

[Qemu-devel] [PULL 4/8] qemu-nbd: Use program name in error messages

[Eric Blake]

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 <eblake@redhat.com>
Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
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 ca7109652e5..e169b839ece 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 94acd9b9479..5f416721b03 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 ==
-- 
2.20.1

[Qemu-devel] [PULL 5/8] nbd: Document timeline of various features

[Eric Blake]

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 <vsementsov@virtuozzo.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <20181215135324.152629-3-eblake@redhat.com>
Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 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 77b5f459111..fc64473e02b 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:<dirty-bitmap-export-name>":
             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
-- 
2.20.1

[Qemu-devel] [PULL 6/8] nbd/client: More consistent error messages

[Eric Blake]

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 <eblake@redhat.com>
Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
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 0ad7147ed95..e77414711ba 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;
     }
-- 
2.20.1

[Qemu-devel] [PULL 7/8] qemu-nbd: Fail earlier for -c/-d on non-linux

[Eric Blake]

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 <eblake@redhat.com>
Message-Id: <20181215135324.152629-7-eblake@redhat.com>
Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 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 e77414711ba..5a03a844187 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 e169b839ece..2807e132396 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));
-- 
2.20.1

[Qemu-devel] [PULL 8/8] nbd/client: Drop pointless buf variable

[Eric Blake]

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 <eblake@redhat.com>
Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-Id: <20181215135324.152629-9-eblake@redhat.com>
---
 nbd/nbd-internal.h |  3 ++-
 nbd/client.c       | 22 +++++++---------------
 2 files changed, 9 insertions(+), 16 deletions(-)

diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h
index f38be9ebaaf..82aa221227f 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

diff --git a/nbd/client.c b/nbd/client.c
index 5a03a844187..f625c207c54 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: ");
+    if (nbd_read(ioc, &magic, sizeof(magic), errp) < 0) {
+        error_prepend(errp, "Failed to read initial magic: ");
         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) {
-        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;
     }

-- 
2.20.1

Re: [Qemu-devel] [PULL 0/8] NBD patches through 2019-01-05

[Peter Maydell]

On Sat, 5 Jan 2019 at 14:03, Eric Blake <eblake@redhat.com> wrote:
>
> The following changes since commit 9b2e891ec5ccdb4a7d583b77988848282606fdea:
>
>   Merge remote-tracking branch 'remotes/marcel/tags/rdma-pull-request' into staging (2018-12-22 11:25:31 +0000)
>
> are available in the Git repository at:
>
>   https://repo.or.cz/qemu/ericb.git tags/pull-nbd-2019-01-05
>
> for you to fetch changes up to ef2e35fcc8e14bcc9366df5fdf53f65d679f8dca:
>
>   nbd/client: Drop pointless buf variable (2019-01-05 07:53:22 -0600)
>
> ----------------------------------------------------------------
> nbd patches for 2019-01-05
>
> Error and trace improvements in NBD code, such as less noise for
> common disconnect scenarios.
>
> - Vladimir Sementsov-Ogievskiy: 0/3 nbd-client: drop extra error noise
> - Eric Blake: portions of 0/22 nbd: add qemu-nbd --list
>

Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/4.0
for any user-visible changes.

-- PMM