[Qemu-devel] [PATCH v4 0/8] nbd mimimal structured read

[Qemu-devel] [PATCH v4 0/8] nbd mimimal structured read

[Eric Blake]

As mentioned in my review of Vladimir's v3 of this series [1],
I had enough tweaks during my review that it's easier to repost
things for another round of discussion, adding some of my patches
in between his.  I did not include his 13/13 "nbd: Minimal structured
read for client", where I had a lot of comments, and suggest that
Vladimir is in the best position to rebase that patch on top of
this v4 series (post it as a 9/8, if desired).

[1] https://lists.gnu.org/archive/html/qemu-devel/2017-10/msg02755.html

Based-on: <20171015004033.3248-1-eblake@redhat.com>
([PULL 0/9] NBD patches through 14 Oct)

Eric Blake (5):
  nbd: Include error names in trace messages
  nbd: Move nbd_errno_to_system_errno() to public header
  nbd: Expose constants and structs for structured read
  nbd/server: Include human-readable message in structured errors
  nbd: Move nbd_read() to common header

Vladimir Sementsov-Ogievskiy (3):
  nbd: Minimal structured read for server
  nbd/client: refactor nbd_receive_starttls
  nbd/client: prepare nbd_receive_reply for structured reply

 include/block/nbd.h |  94 ++++++++++++++++++++++--
 nbd/nbd-internal.h  |  24 +------
 block/nbd-client.c  |   8 ++-
 nbd/client.c        | 204 +++++++++++++++++++++++++++++++++-------------------
 nbd/common.c        |  84 ++++++++++++++++++++++
 nbd/server.c        | 121 ++++++++++++++++++++++++++++---
 nbd/trace-events    |  15 ++--
 7 files changed, 427 insertions(+), 123 deletions(-)

-- 
2.13.6

[Qemu-devel] [PATCH v4 1/8] nbd: Include error names in trace messages

[Eric Blake]

NBD errors were originally sent over the wire based on Linux errno
values; but not all the world is Linux, and not all platforms share
the same values.  Since a number isn't very easy to decipher on all
platforms, update the trace messages to include the name of NBD
errors being sent/received over the wire.  Tweak the trace messages
to be at the point where we are using the NBD error, not the
translation to the host errno values.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 nbd/nbd-internal.h |  1 +
 nbd/client.c       |  3 ++-
 nbd/common.c       | 23 +++++++++++++++++++++++
 nbd/server.c       |  3 ++-
 nbd/trace-events   |  4 ++--
 5 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h
index 11a130d050..4bfe5be884 100644
--- a/nbd/nbd-internal.h
+++ b/nbd/nbd-internal.h
@@ -126,6 +126,7 @@ 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);

diff --git a/nbd/client.c b/nbd/client.c
index cd5a2c80ac..59d7c9d49f 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -940,6 +940,8 @@ int nbd_receive_reply(QIOChannel *ioc, NBDReply *reply, Error **errp)
     reply->error  = ldl_be_p(buf + 4);
     reply->handle = ldq_be_p(buf + 8);

+    trace_nbd_receive_reply(magic, reply->error, nbd_err_lookup(reply->error),
+                            reply->handle);
     reply->error = nbd_errno_to_system_errno(reply->error);

     if (reply->error == ESHUTDOWN) {
@@ -947,7 +949,6 @@ int nbd_receive_reply(QIOChannel *ioc, NBDReply *reply, Error **errp)
         error_setg(errp, "server shutting down");
         return -EINVAL;
     }
-    trace_nbd_receive_reply(magic, reply->error, reply->handle);

     if (magic != NBD_SIMPLE_REPLY_MAGIC) {
         error_setg(errp, "invalid magic (got 0x%" PRIx32 ")", magic);
diff --git a/nbd/common.c b/nbd/common.c
index 59a5316be9..7456021f7e 100644
--- a/nbd/common.c
+++ b/nbd/common.c
@@ -148,3 +148,26 @@ const char *nbd_cmd_lookup(uint16_t cmd)
         return "<unknown>";
     }
 }
+
+
+const char *nbd_err_lookup(int err)
+{
+    switch (err) {
+    case NBD_SUCCESS:
+        return "success";
+    case NBD_EPERM:
+        return "EPERM";
+    case NBD_EIO:
+        return "EIO";
+    case NBD_ENOMEM:
+        return "ENOMEM";
+    case NBD_EINVAL:
+        return "EINVAL";
+    case NBD_ENOSPC:
+        return "ENOSPC";
+    case NBD_ESHUTDOWN:
+        return "ESHUTDOWN";
+    default:
+        return "<unknown>";
+    }
+}
diff --git a/nbd/server.c b/nbd/server.c
index 3df3548d6d..459e00c553 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1227,7 +1227,8 @@ static int nbd_co_send_simple_reply(NBDClient *client,
         {.iov_base = data, .iov_len = len}
     };

-    trace_nbd_co_send_simple_reply(handle, nbd_err, len);
+    trace_nbd_co_send_simple_reply(handle, nbd_err, nbd_err_lookup(nbd_err),
+                                   len);
     set_be_simple_reply(&reply, nbd_err, handle);

     return nbd_co_send_iov(client, iov, len ? 2 : 1, errp);
diff --git a/nbd/trace-events b/nbd/trace-events
index e27614f050..9a72f458b2 100644
--- a/nbd/trace-events
+++ b/nbd/trace-events
@@ -29,7 +29,7 @@ nbd_client_loop_ret(int ret, const char *error) "NBD loop returned %d: %s"
 nbd_client_clear_queue(void) "Clearing NBD queue"
 nbd_client_clear_socket(void) "Clearing NBD socket"
 nbd_send_request(uint64_t from, uint32_t len, uint64_t handle, uint16_t flags, uint16_t type, const char *name) "Sending request to server: { .from = %" PRIu64", .len = %" PRIu32 ", .handle = %" PRIu64 ", .flags = 0x%" PRIx16 ", .type = %" PRIu16 " (%s) }"
-nbd_receive_reply(uint32_t magic, int32_t error, uint64_t handle) "Got reply: { magic = 0x%" PRIx32 ", .error = % " PRId32 ", handle = %" PRIu64" }"
+nbd_receive_reply(uint32_t magic, int32_t error, const char *errname, uint64_t handle) "Got reply: { magic = 0x%" PRIx32 ", .error = % " PRId32 " (%s), handle = %" PRIu64" }"

 # nbd/server.c
 nbd_negotiate_send_rep_len(uint32_t opt, const char *optname, uint32_t type, const char *typename, uint32_t len) "Reply opt=0x%" PRIx32 " (%s), type=0x%" PRIx32 " (%s), len=%" PRIu32
@@ -53,7 +53,7 @@ nbd_negotiate_success(void) "Negotiation succeeded"
 nbd_receive_request(uint32_t magic, uint16_t flags, uint16_t type, uint64_t from, uint32_t len) "Got request: { magic = 0x%" PRIx32 ", .flags = 0x%" PRIx16 ", .type = 0x%" PRIx16 ", from = %" PRIu64 ", len = %" PRIu32 " }"
 nbd_blk_aio_attached(const char *name, void *ctx) "Export %s: Attaching clients to AIO context %p\n"
 nbd_blk_aio_detach(const char *name, void *ctx) "Export %s: Detaching clients from AIO context %p\n"
-nbd_co_send_simple_reply(uint64_t handle, uint32_t error, int len) "Send simple reply: handle = %" PRIu64 ", error = %" PRIu32 ", len = %d"
+nbd_co_send_simple_reply(uint64_t handle, uint32_t error, const char *errname, int len) "Send simple reply: handle = %" PRIu64 ", error = %" PRIu32 " (%s), len = %d"
 nbd_co_receive_request_decode_type(uint64_t handle, uint16_t type, const char *name) "Decoding type: handle = %" PRIu64 ", type = %" PRIu16 " (%s)"
 nbd_co_receive_request_payload_received(uint64_t handle, uint32_t len) "Payload received: handle = %" PRIu64 ", len = %" PRIu32
 nbd_co_receive_request_cmd_write(uint32_t len) "Reading %" PRIu32 " byte(s)"
-- 
2.13.6

Re: [Qemu-devel] [PATCH v4 1/8] nbd: Include error names in trace messages

[Vladimir Sementsov-Ogievskiy]

15.10.2017 04:01, Eric Blake wrote:
> NBD errors were originally sent over the wire based on Linux errno
> values; but not all the world is Linux, and not all platforms share
> the same values.  Since a number isn't very easy to decipher on all
> platforms, update the trace messages to include the name of NBD
> errors being sent/received over the wire.  Tweak the trace messages
> to be at the point where we are using the NBD error, not the
> translation to the host errno values.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>

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


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v4 1/8] nbd: Include error names in trace messages

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1624 bytes --]

On 10/14/2017 08:01 PM, Eric Blake wrote:
> NBD errors were originally sent over the wire based on Linux errno
> values; but not all the world is Linux, and not all platforms share
> the same values.  Since a number isn't very easy to decipher on all
> platforms, update the trace messages to include the name of NBD
> errors being sent/received over the wire.  Tweak the trace messages
> to be at the point where we are using the NBD error, not the
> translation to the host errno values.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---

> +++ b/nbd/trace-events
> @@ -29,7 +29,7 @@ nbd_client_loop_ret(int ret, const char *error) "NBD loop returned %d: %s"
>  nbd_client_clear_queue(void) "Clearing NBD queue"
>  nbd_client_clear_socket(void) "Clearing NBD socket"
>  nbd_send_request(uint64_t from, uint32_t len, uint64_t handle, uint16_t flags, uint16_t type, const char *name) "Sending request to server: { .from = %" PRIu64", .len = %" PRIu32 ", .handle = %" PRIu64 ", .flags = 0x%" PRIx16 ", .type = %" PRIu16 " (%s) }"
> -nbd_receive_reply(uint32_t magic, int32_t error, uint64_t handle) "Got reply: { magic = 0x%" PRIx32 ", .error = % " PRId32 ", handle = %" PRIu64" }"

Pre-existing unusual spacing in '.error = % " PRId32',

> +nbd_receive_reply(uint32_t magic, int32_t error, const char *errname, uint64_t handle) "Got reply: { magic = 0x%" PRIx32 ", .error = % " PRId32 " (%s), handle = %" PRIu64" }"

so I'll clean that up in the next revision.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

[Qemu-devel] [PATCH v4 2/8] nbd: Move nbd_errno_to_system_errno() to public header

[Eric Blake]

This is needed in preparation for structured reply handling,
as we will be performing the translation from NBD error to
system errno value higher in the stack at block/nbd-client.c.
---
 include/block/nbd.h | 13 +++++++++++++
 nbd/nbd-internal.h  | 12 ------------
 nbd/client.c        | 32 --------------------------------
 nbd/common.c        | 34 ++++++++++++++++++++++++++++++++++
 nbd/trace-events    |  4 +++-
 5 files changed, 50 insertions(+), 45 deletions(-)

diff --git a/include/block/nbd.h b/include/block/nbd.h
index a6df5ce8b5..dc62b5cd19 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -149,6 +149,18 @@ enum {
  * aren't overflowing some other buffer. */
 #define NBD_MAX_NAME_SIZE 256

+/* NBD errors are based on errno numbers, so there is a 1:1 mapping,
+ * but only a limited set of errno values is specified in the protocol.
+ * Everything else is squashed to EINVAL.
+ */
+#define NBD_SUCCESS    0
+#define NBD_EPERM      1
+#define NBD_EIO        5
+#define NBD_ENOMEM     12
+#define NBD_EINVAL     22
+#define NBD_ENOSPC     28
+#define NBD_ESHUTDOWN  108
+
 /* Details collected by NBD_OPT_EXPORT_NAME and NBD_OPT_GO */
 struct NBDExportInfo {
     /* Set by client before nbd_receive_negotiate() */
@@ -172,6 +184,7 @@ int nbd_send_request(QIOChannel *ioc, NBDRequest *request);
 int nbd_receive_reply(QIOChannel *ioc, NBDReply *reply, Error **errp);
 int nbd_client(int fd);
 int nbd_disconnect(int fd);
+int nbd_errno_to_system_errno(int err);

 typedef struct NBDExport NBDExport;
 typedef struct NBDClient NBDClient;
diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h
index 4bfe5be884..df6c8b2f24 100644
--- a/nbd/nbd-internal.h
+++ b/nbd/nbd-internal.h
@@ -64,18 +64,6 @@
 #define NBD_SET_TIMEOUT             _IO(0xab, 9)
 #define NBD_SET_FLAGS               _IO(0xab, 10)

-/* NBD errors are based on errno numbers, so there is a 1:1 mapping,
- * but only a limited set of errno values is specified in the protocol.
- * Everything else is squashed to EINVAL.
- */
-#define NBD_SUCCESS    0
-#define NBD_EPERM      1
-#define NBD_EIO        5
-#define NBD_ENOMEM     12
-#define NBD_EINVAL     22
-#define NBD_ENOSPC     28
-#define NBD_ESHUTDOWN  108
-
 /* nbd_read_eof
  * Tries to read @size bytes from @ioc.
  * Returns 1 on success
diff --git a/nbd/client.c b/nbd/client.c
index 59d7c9d49f..50f36b511e 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -22,38 +22,6 @@
 #include "trace.h"
 #include "nbd-internal.h"

-static int nbd_errno_to_system_errno(int err)
-{
-    int ret;
-    switch (err) {
-    case NBD_SUCCESS:
-        ret = 0;
-        break;
-    case NBD_EPERM:
-        ret = EPERM;
-        break;
-    case NBD_EIO:
-        ret = EIO;
-        break;
-    case NBD_ENOMEM:
-        ret = ENOMEM;
-        break;
-    case NBD_ENOSPC:
-        ret = ENOSPC;
-        break;
-    case NBD_ESHUTDOWN:
-        ret = ESHUTDOWN;
-        break;
-    default:
-        trace_nbd_unknown_error(err);
-        /* fallthrough */
-    case NBD_EINVAL:
-        ret = EINVAL;
-        break;
-    }
-    return ret;
-}
-
 /* Definitions for opaque data types */

 static QTAILQ_HEAD(, NBDExport) exports = QTAILQ_HEAD_INITIALIZER(exports);
diff --git a/nbd/common.c b/nbd/common.c
index 7456021f7e..593904f148 100644
--- a/nbd/common.c
+++ b/nbd/common.c
@@ -18,6 +18,7 @@

 #include "qemu/osdep.h"
 #include "qapi/error.h"
+#include "trace.h"
 #include "nbd-internal.h"

 /* Discard length bytes from channel.  Return -errno on failure and 0 on
@@ -171,3 +172,36 @@ const char *nbd_err_lookup(int err)
         return "<unknown>";
     }
 }
+
+
+int nbd_errno_to_system_errno(int err)
+{
+    int ret;
+    switch (err) {
+    case NBD_SUCCESS:
+        ret = 0;
+        break;
+    case NBD_EPERM:
+        ret = EPERM;
+        break;
+    case NBD_EIO:
+        ret = EIO;
+        break;
+    case NBD_ENOMEM:
+        ret = ENOMEM;
+        break;
+    case NBD_ENOSPC:
+        ret = ENOSPC;
+        break;
+    case NBD_ESHUTDOWN:
+        ret = ESHUTDOWN;
+        break;
+    default:
+        trace_nbd_unknown_error(err);
+        /* fallthrough */
+    case NBD_EINVAL:
+        ret = EINVAL;
+        break;
+    }
+    return ret;
+}
diff --git a/nbd/trace-events b/nbd/trace-events
index 9a72f458b2..d3b702dd9a 100644
--- a/nbd/trace-events
+++ b/nbd/trace-events
@@ -1,5 +1,4 @@
 # nbd/client.c
-nbd_unknown_error(int err) "Squashing unexpected error %d to EINVAL"
 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 0x%" PRIx32" (%s), type 0x%" PRIx32" (%s), len %" PRIu32
 nbd_reply_err_unsup(uint32_t option, const char *name) "server doesn't understand request 0x%" PRIx32 " (%s), attempting fallback"
@@ -31,6 +30,9 @@ nbd_client_clear_socket(void) "Clearing NBD socket"
 nbd_send_request(uint64_t from, uint32_t len, uint64_t handle, uint16_t flags, uint16_t type, const char *name) "Sending request to server: { .from = %" PRIu64", .len = %" PRIu32 ", .handle = %" PRIu64 ", .flags = 0x%" PRIx16 ", .type = %" PRIu16 " (%s) }"
 nbd_receive_reply(uint32_t magic, int32_t error, const char *errname, uint64_t handle) "Got reply: { magic = 0x%" PRIx32 ", .error = % " PRId32 " (%s), handle = %" PRIu64" }"

+# nbd/common.c
+nbd_unknown_error(int err) "Squashing unexpected error %d to EINVAL"
+
 # nbd/server.c
 nbd_negotiate_send_rep_len(uint32_t opt, const char *optname, uint32_t type, const char *typename, uint32_t len) "Reply opt=0x%" PRIx32 " (%s), type=0x%" PRIx32 " (%s), len=%" PRIu32
 nbd_negotiate_send_rep_err(const char *msg) "sending error message \"%s\""
-- 
2.13.6

Re: [Qemu-devel] [PATCH v4 2/8] nbd: Move nbd_errno_to_system_errno() to public header

[Vladimir Sementsov-Ogievskiy]

15.10.2017 04:01, Eric Blake wrote:
> This is needed in preparation for structured reply handling,
> as we will be performing the translation from NBD error to
> system errno value higher in the stack at block/nbd-client.c.

you've forget to sign-off.

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

> ---
>   include/block/nbd.h | 13 +++++++++++++
>   nbd/nbd-internal.h  | 12 ------------
>   nbd/client.c        | 32 --------------------------------
>   nbd/common.c        | 34 ++++++++++++++++++++++++++++++++++
>   nbd/trace-events    |  4 +++-
>   5 files changed, 50 insertions(+), 45 deletions(-)
>
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index a6df5ce8b5..dc62b5cd19 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -149,6 +149,18 @@ enum {
>    * aren't overflowing some other buffer. */
>   #define NBD_MAX_NAME_SIZE 256
>
> +/* NBD errors are based on errno numbers, so there is a 1:1 mapping,
> + * but only a limited set of errno values is specified in the protocol.
> + * Everything else is squashed to EINVAL.
> + */
> +#define NBD_SUCCESS    0
> +#define NBD_EPERM      1
> +#define NBD_EIO        5
> +#define NBD_ENOMEM     12
> +#define NBD_EINVAL     22
> +#define NBD_ENOSPC     28
> +#define NBD_ESHUTDOWN  108
> +
>   /* Details collected by NBD_OPT_EXPORT_NAME and NBD_OPT_GO */
>   struct NBDExportInfo {
>       /* Set by client before nbd_receive_negotiate() */
> @@ -172,6 +184,7 @@ int nbd_send_request(QIOChannel *ioc, NBDRequest *request);
>   int nbd_receive_reply(QIOChannel *ioc, NBDReply *reply, Error **errp);
>   int nbd_client(int fd);
>   int nbd_disconnect(int fd);
> +int nbd_errno_to_system_errno(int err);
>
>   typedef struct NBDExport NBDExport;
>   typedef struct NBDClient NBDClient;
> diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h
> index 4bfe5be884..df6c8b2f24 100644
> --- a/nbd/nbd-internal.h
> +++ b/nbd/nbd-internal.h
> @@ -64,18 +64,6 @@
>   #define NBD_SET_TIMEOUT             _IO(0xab, 9)
>   #define NBD_SET_FLAGS               _IO(0xab, 10)
>
> -/* NBD errors are based on errno numbers, so there is a 1:1 mapping,
> - * but only a limited set of errno values is specified in the protocol.
> - * Everything else is squashed to EINVAL.
> - */
> -#define NBD_SUCCESS    0
> -#define NBD_EPERM      1
> -#define NBD_EIO        5
> -#define NBD_ENOMEM     12
> -#define NBD_EINVAL     22
> -#define NBD_ENOSPC     28
> -#define NBD_ESHUTDOWN  108
> -
>   /* nbd_read_eof
>    * Tries to read @size bytes from @ioc.
>    * Returns 1 on success
> diff --git a/nbd/client.c b/nbd/client.c
> index 59d7c9d49f..50f36b511e 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -22,38 +22,6 @@
>   #include "trace.h"
>   #include "nbd-internal.h"
>
> -static int nbd_errno_to_system_errno(int err)
> -{
> -    int ret;
> -    switch (err) {
> -    case NBD_SUCCESS:
> -        ret = 0;
> -        break;
> -    case NBD_EPERM:
> -        ret = EPERM;
> -        break;
> -    case NBD_EIO:
> -        ret = EIO;
> -        break;
> -    case NBD_ENOMEM:
> -        ret = ENOMEM;
> -        break;
> -    case NBD_ENOSPC:
> -        ret = ENOSPC;
> -        break;
> -    case NBD_ESHUTDOWN:
> -        ret = ESHUTDOWN;
> -        break;
> -    default:
> -        trace_nbd_unknown_error(err);
> -        /* fallthrough */
> -    case NBD_EINVAL:
> -        ret = EINVAL;
> -        break;
> -    }
> -    return ret;
> -}
> -
>   /* Definitions for opaque data types */
>
>   static QTAILQ_HEAD(, NBDExport) exports = QTAILQ_HEAD_INITIALIZER(exports);
> diff --git a/nbd/common.c b/nbd/common.c
> index 7456021f7e..593904f148 100644
> --- a/nbd/common.c
> +++ b/nbd/common.c
> @@ -18,6 +18,7 @@
>
>   #include "qemu/osdep.h"
>   #include "qapi/error.h"
> +#include "trace.h"
>   #include "nbd-internal.h"
>
>   /* Discard length bytes from channel.  Return -errno on failure and 0 on
> @@ -171,3 +172,36 @@ const char *nbd_err_lookup(int err)
>           return "<unknown>";
>       }
>   }
> +
> +
> +int nbd_errno_to_system_errno(int err)
> +{
> +    int ret;
> +    switch (err) {
> +    case NBD_SUCCESS:
> +        ret = 0;
> +        break;
> +    case NBD_EPERM:
> +        ret = EPERM;
> +        break;
> +    case NBD_EIO:
> +        ret = EIO;
> +        break;
> +    case NBD_ENOMEM:
> +        ret = ENOMEM;
> +        break;
> +    case NBD_ENOSPC:
> +        ret = ENOSPC;
> +        break;
> +    case NBD_ESHUTDOWN:
> +        ret = ESHUTDOWN;
> +        break;
> +    default:
> +        trace_nbd_unknown_error(err);
> +        /* fallthrough */
> +    case NBD_EINVAL:
> +        ret = EINVAL;
> +        break;
> +    }
> +    return ret;
> +}
> diff --git a/nbd/trace-events b/nbd/trace-events
> index 9a72f458b2..d3b702dd9a 100644
> --- a/nbd/trace-events
> +++ b/nbd/trace-events
> @@ -1,5 +1,4 @@
>   # nbd/client.c
> -nbd_unknown_error(int err) "Squashing unexpected error %d to EINVAL"
>   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 0x%" PRIx32" (%s), type 0x%" PRIx32" (%s), len %" PRIu32
>   nbd_reply_err_unsup(uint32_t option, const char *name) "server doesn't understand request 0x%" PRIx32 " (%s), attempting fallback"
> @@ -31,6 +30,9 @@ nbd_client_clear_socket(void) "Clearing NBD socket"
>   nbd_send_request(uint64_t from, uint32_t len, uint64_t handle, uint16_t flags, uint16_t type, const char *name) "Sending request to server: { .from = %" PRIu64", .len = %" PRIu32 ", .handle = %" PRIu64 ", .flags = 0x%" PRIx16 ", .type = %" PRIu16 " (%s) }"
>   nbd_receive_reply(uint32_t magic, int32_t error, const char *errname, uint64_t handle) "Got reply: { magic = 0x%" PRIx32 ", .error = % " PRId32 " (%s), handle = %" PRIu64" }"
>
> +# nbd/common.c
> +nbd_unknown_error(int err) "Squashing unexpected error %d to EINVAL"
> +
>   # nbd/server.c
>   nbd_negotiate_send_rep_len(uint32_t opt, const char *optname, uint32_t type, const char *typename, uint32_t len) "Reply opt=0x%" PRIx32 " (%s), type=0x%" PRIx32 " (%s), len=%" PRIu32
>   nbd_negotiate_send_rep_err(const char *msg) "sending error message \"%s\""


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v4 2/8] nbd: Move nbd_errno_to_system_errno() to public header

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 977 bytes --]

On 10/16/2017 03:33 AM, Vladimir Sementsov-Ogievskiy wrote:
> 15.10.2017 04:01, Eric Blake wrote:
>> This is needed in preparation for structured reply handling,
>> as we will be performing the translation from NBD error to
>> system errno value higher in the stack at block/nbd-client.c.
> 
> you've forget to sign-off.

D'oh. Bad patch split on my part.

Signed-off-by: Eric Blake <eblake@redhat.com>

> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
>> ---
>>   include/block/nbd.h | 13 +++++++++++++
>>   nbd/nbd-internal.h  | 12 ------------
>>   nbd/client.c        | 32 --------------------------------
>>   nbd/common.c        | 34 ++++++++++++++++++++++++++++++++++
>>   nbd/trace-events    |  4 +++-
>>   5 files changed, 50 insertions(+), 45 deletions(-)
>>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

[Qemu-devel] [PATCH v4 3/8] nbd: Expose constants and structs for structured read

[Eric Blake]

Upcoming patches will implement the NBD structured reply
extension [1] for both client and server roles.  Declare the
constants, structs, and lookup routines that will be valuable
whether the server or client code is backported in isolation.

This includes moving one constant from an internal header to
the public header, as part of the structured read processing
will be done in block/nbd-client.c rather than nbd/client.c.

[1]https://github.com/NetworkBlockDevice/nbd/blob/extension-structured-reply/doc/proto.md

Based on patches from Vladimir Sementsov-Ogievskiy.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 include/block/nbd.h | 41 +++++++++++++++++++++++++++++++++++++++++
 nbd/nbd-internal.h  |  2 +-
 nbd/common.c        | 27 +++++++++++++++++++++++++++
 nbd/server.c        |  2 ++
 4 files changed, 71 insertions(+), 1 deletion(-)

diff --git a/include/block/nbd.h b/include/block/nbd.h
index dc62b5cd19..225e9575e4 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -69,6 +69,28 @@ typedef struct NBDSimpleReply {
     uint64_t handle;
 } QEMU_PACKED NBDSimpleReply;

+/* Header of all structured replies */
+typedef struct NBDStructuredReplyChunk {
+    uint32_t magic;  /* NBD_STRUCTURED_REPLY_MAGIC */
+    uint16_t flags;  /* combination of NBD_REPLY_FLAG_* */
+    uint16_t type;   /* NBD_REPLY_TYPE_* */
+    uint64_t handle; /* request handle */
+    uint32_t length; /* length of payload */
+} QEMU_PACKED NBDStructuredReplyChunk;
+
+/* Header of NBD_REPLY_TYPE_OFFSET_DATA, complete NBD_REPLY_TYPE_OFFSET_HOLE */
+typedef struct NBDStructuredRead {
+    NBDStructuredReplyChunk h;
+    uint64_t offset;
+} QEMU_PACKED NBDStructuredRead;
+
+/* Header of all NBD_REPLY_TYPE_ERROR* errors */
+typedef struct NBDStructuredError {
+    NBDStructuredReplyChunk h;
+    uint32_t error;
+    uint16_t message_length;
+} QEMU_PACKED NBDStructuredError;
+
 /* Transmission (export) flags: sent from server to client during handshake,
    but describe what will happen during transmission */
 #define NBD_FLAG_HAS_FLAGS         (1 << 0) /* Flags are there */
@@ -79,6 +101,7 @@ typedef struct NBDSimpleReply {
                                                rotational media */
 #define NBD_FLAG_SEND_TRIM         (1 << 5) /* Send TRIM (discard) */
 #define NBD_FLAG_SEND_WRITE_ZEROES (1 << 6) /* Send WRITE_ZEROES */
+#define NBD_FLAG_SEND_DF           (1 << 7) /* Send DF (Do not Fragment) */

 /* New-style handshake (global) flags, sent from server to client, and
    control what will happen during handshake phase. */
@@ -125,6 +148,7 @@ typedef struct NBDSimpleReply {
 /* Request flags, sent from client to server during transmission phase */
 #define NBD_CMD_FLAG_FUA        (1 << 0) /* 'force unit access' during write */
 #define NBD_CMD_FLAG_NO_HOLE    (1 << 1) /* don't punch hole on zero run */
+#define NBD_CMD_FLAG_DF         (1 << 2) /* don't fragment structured read */

 /* Supported request types */
 enum {
@@ -149,6 +173,22 @@ enum {
  * aren't overflowing some other buffer. */
 #define NBD_MAX_NAME_SIZE 256

+/* Two types of reply structures */
+#define NBD_SIMPLE_REPLY_MAGIC      0x67446698
+#define NBD_STRUCTURED_REPLY_MAGIC  0x668e33ef
+
+/* Structured reply flags */
+#define NBD_REPLY_FLAG_DONE          (1 << 0) /* This reply-chunk is last */
+
+/* Structured reply types */
+#define NBD_REPLY_ERR(value)         ((1 << 15) | (value))
+
+#define NBD_REPLY_TYPE_NONE          0
+#define NBD_REPLY_TYPE_OFFSET_DATA   1
+#define NBD_REPLY_TYPE_OFFSET_HOLE   2
+#define NBD_REPLY_TYPE_ERROR         NBD_REPLY_ERR(1)
+#define NBD_REPLY_TYPE_ERROR_OFFSET  NBD_REPLY_ERR(2)
+
 /* NBD errors are based on errno numbers, so there is a 1:1 mapping,
  * but only a limited set of errno values is specified in the protocol.
  * Everything else is squashed to EINVAL.
@@ -159,6 +199,7 @@ enum {
 #define NBD_ENOMEM     12
 #define NBD_EINVAL     22
 #define NBD_ENOSPC     28
+#define NBD_EOVERFLOW  75
 #define NBD_ESHUTDOWN  108

 /* Details collected by NBD_OPT_EXPORT_NAME and NBD_OPT_GO */
diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h
index df6c8b2f24..4f24d6e57d 100644
--- a/nbd/nbd-internal.h
+++ b/nbd/nbd-internal.h
@@ -47,7 +47,6 @@
 #define NBD_OLDSTYLE_NEGOTIATE_SIZE (8 + 8 + 8 + 4 + 124)

 #define NBD_REQUEST_MAGIC           0x25609513
-#define NBD_SIMPLE_REPLY_MAGIC      0x67446698
 #define NBD_OPTS_MAGIC              0x49484156454F5054LL
 #define NBD_CLIENT_MAGIC            0x0000420281861253LL
 #define NBD_REP_MAGIC               0x0003e889045565a9LL
@@ -114,6 +113,7 @@ 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_reply_type_lookup(uint16_t type);
 const char *nbd_err_lookup(int err);

 int nbd_drop(QIOChannel *ioc, size_t size, Error **errp);
diff --git a/nbd/common.c b/nbd/common.c
index 593904f148..6047d71748 100644
--- a/nbd/common.c
+++ b/nbd/common.c
@@ -151,6 +151,28 @@ const char *nbd_cmd_lookup(uint16_t cmd)
 }


+const char *nbd_reply_type_lookup(uint16_t type)
+{
+    switch (type) {
+    case NBD_REPLY_TYPE_NONE:
+        return "none";
+    case NBD_REPLY_TYPE_OFFSET_DATA:
+        return "data";
+    case NBD_REPLY_TYPE_OFFSET_HOLE:
+        return "hole";
+    case NBD_REPLY_TYPE_ERROR:
+        return "generic error";
+    case NBD_REPLY_TYPE_ERROR_OFFSET:
+        return "error at offset";
+    default:
+        if (type & (1 << 15)) {
+            return "<unknown error>";
+        }
+        return "<unknown>";
+    }
+}
+
+
 const char *nbd_err_lookup(int err)
 {
     switch (err) {
@@ -166,6 +188,8 @@ const char *nbd_err_lookup(int err)
         return "EINVAL";
     case NBD_ENOSPC:
         return "ENOSPC";
+    case NBD_EOVERFLOW:
+        return "EOVERFLOW";
     case NBD_ESHUTDOWN:
         return "ESHUTDOWN";
     default:
@@ -193,6 +217,9 @@ int nbd_errno_to_system_errno(int err)
     case NBD_ENOSPC:
         ret = ENOSPC;
         break;
+    case NBD_EOVERFLOW:
+        ret = EOVERFLOW;
+        break;
     case NBD_ESHUTDOWN:
         ret = ESHUTDOWN;
         break;
diff --git a/nbd/server.c b/nbd/server.c
index 459e00c553..efb6003364 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -40,6 +40,8 @@ static int system_errno_to_nbd_errno(int err)
     case EFBIG:
     case ENOSPC:
         return NBD_ENOSPC;
+    case EOVERFLOW:
+        return NBD_EOVERFLOW;
     case ESHUTDOWN:
         return NBD_ESHUTDOWN;
     case EINVAL:
-- 
2.13.6

Re: [Qemu-devel] [PATCH v4 3/8] nbd: Expose constants and structs for structured read

[Vladimir Sementsov-Ogievskiy]

15.10.2017 04:01, Eric Blake wrote:
> Upcoming patches will implement the NBD structured reply
> extension [1] for both client and server roles.  Declare the
> constants, structs, and lookup routines that will be valuable
> whether the server or client code is backported in isolation.
>
> This includes moving one constant from an internal header to
> the public header, as part of the structured read processing
> will be done in block/nbd-client.c rather than nbd/client.c.
>
> [1]https://github.com/NetworkBlockDevice/nbd/blob/extension-structured-reply/doc/proto.md

this link may become dead in future, after merging extension spec into 
master...

>
> Based on patches from Vladimir Sementsov-Ogievskiy.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>

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



-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v4 3/8] nbd: Expose constants and structs for structured read

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1434 bytes --]

On 10/16/2017 03:49 AM, Vladimir Sementsov-Ogievskiy wrote:
> 15.10.2017 04:01, Eric Blake wrote:
>> Upcoming patches will implement the NBD structured reply
>> extension [1] for both client and server roles.  Declare the
>> constants, structs, and lookup routines that will be valuable
>> whether the server or client code is backported in isolation.
>>
>> This includes moving one constant from an internal header to
>> the public header, as part of the structured read processing
>> will be done in block/nbd-client.c rather than nbd/client.c.
>>
>> [1]https://github.com/NetworkBlockDevice/nbd/blob/extension-structured-reply/doc/proto.md
>>
> 
> this link may become dead in future, after merging extension spec into
> master...

Perhaps; but it's a git branch, so it's not going to disappear, even
when it is no longer actively maintained.  For comparison, here's the
(now-stale) link for NBD_CMD_WRITE_ZEROES:

https://github.com/NetworkBlockDevice/nbd/blob/extension-write-zeroes/doc/proto.md

So I'm not too worried about having the URL in the commit message.

> 
>>
>> Based on patches from Vladimir Sementsov-Ogievskiy.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> 
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

[Qemu-devel] [PATCH v4 4/8] nbd: Minimal structured read for server

[Eric Blake]

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

Minimal implementation of structured read: one structured reply chunk,
no segmentation.
Minimal structured error implementation: no text message.
Support DF flag, but just ignore it, as there is no segmentation any
way.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Eric Blake <eblake@redhat.com>

---
v4: better _DF flag handling, convert errno to wire format, add
comments and tracing, rework structured error for less churn when adding
text message later, don't kill connection on redundant client option
---
 nbd/server.c     | 106 +++++++++++++++++++++++++++++++++++++++++++++++++------
 nbd/trace-events |   2 ++
 2 files changed, 98 insertions(+), 10 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index efb6003364..23dc6be708 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -100,6 +100,8 @@ struct NBDClient {
     QTAILQ_ENTRY(NBDClient) next;
     int nb_requests;
     bool closing;
+
+    bool structured_reply;
 };

 /* That's all folks */
@@ -762,6 +764,23 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags,
                     return ret;
                 }
                 break;
+
+            case NBD_OPT_STRUCTURED_REPLY:
+                if (client->structured_reply) {
+                    ret = nbd_negotiate_send_rep_err(
+                        client->ioc, NBD_REP_ERR_INVALID, option, errp,
+                        "structured reply already negotiated");
+                } else {
+                    ret = nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK,
+                                                 option, errp);
+                }
+                if (ret < 0) {
+                    return ret;
+                }
+                client->structured_reply = true;
+                myflags |= NBD_CMD_FLAG_DF;
+                break;
+
             default:
                 if (nbd_drop(client->ioc, length, errp) < 0) {
                     return -EIO;
@@ -1236,6 +1255,59 @@ static int nbd_co_send_simple_reply(NBDClient *client,
     return nbd_co_send_iov(client, iov, len ? 2 : 1, errp);
 }

+static inline void set_be_chunk(NBDStructuredReplyChunk *chunk, uint16_t flags,
+                                uint16_t type, uint64_t handle, uint32_t length)
+{
+    stl_be_p(&chunk->magic, NBD_STRUCTURED_REPLY_MAGIC);
+    stw_be_p(&chunk->flags, flags);
+    stw_be_p(&chunk->type, type);
+    stq_be_p(&chunk->handle, handle);
+    stl_be_p(&chunk->length, length);
+}
+
+static int coroutine_fn nbd_co_send_structured_read(NBDClient *client,
+                                                    uint64_t handle,
+                                                    uint64_t offset,
+                                                    void *data,
+                                                    size_t size,
+                                                    Error **errp)
+{
+    NBDStructuredRead chunk;
+    struct iovec iov[] = {
+        {.iov_base = &chunk, .iov_len = sizeof(chunk)},
+        {.iov_base = data, .iov_len = size}
+    };
+
+    trace_nbd_co_send_structured_read(handle, offset, data, size);
+    set_be_chunk(&chunk.h, NBD_REPLY_FLAG_DONE, NBD_REPLY_TYPE_OFFSET_DATA,
+                 handle, sizeof(chunk) - sizeof(chunk.h) + size);
+    stq_be_p(&chunk.offset, offset);
+
+    return nbd_co_send_iov(client, iov, 2, errp);
+}
+
+static int coroutine_fn nbd_co_send_structured_error(NBDClient *client,
+                                                     uint64_t handle,
+                                                     uint32_t error,
+                                                     Error **errp)
+{
+    NBDStructuredError chunk;
+    int nbd_err = system_errno_to_nbd_errno(error);
+    struct iovec iov[] = {
+        {.iov_base = &chunk, .iov_len = sizeof(chunk)},
+        /* FIXME: Support human-readable error message */
+    };
+
+    assert(nbd_err);
+    trace_nbd_co_send_structured_error(handle, nbd_err);
+    set_be_chunk(&chunk.h, NBD_REPLY_FLAG_DONE, NBD_REPLY_TYPE_ERROR, handle,
+                 sizeof(chunk) - sizeof(chunk.h));
+    stl_be_p(&chunk.error, nbd_err);
+    stw_be_p(&chunk.message_length, 0);
+
+    return nbd_co_send_iov(client, iov, 1, errp);
+}
+
 /* nbd_co_receive_request
  * Collect a client request. Return 0 if request looks valid, -EIO to drop
  * connection right away, and any other negative value to report an error to
@@ -1246,6 +1318,7 @@ static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request,
                                   Error **errp)
 {
     NBDClient *client = req->client;
+    int valid_flags;

     g_assert(qemu_in_coroutine());
     assert(client->recv_coroutine == qemu_coroutine_self());
@@ -1307,13 +1380,15 @@ static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request,
                    (uint64_t)client->exp->size);
         return request->type == NBD_CMD_WRITE ? -ENOSPC : -EINVAL;
     }
-    if (request->flags & ~(NBD_CMD_FLAG_FUA | NBD_CMD_FLAG_NO_HOLE)) {
-        error_setg(errp, "unsupported flags (got 0x%x)", request->flags);
-        return -EINVAL;
+    valid_flags = NBD_CMD_FLAG_FUA;
+    if (request->type == NBD_CMD_READ && client->structured_reply) {
+        valid_flags |= NBD_CMD_FLAG_DF;
+    } else if (request->type == NBD_CMD_WRITE_ZEROES) {
+        valid_flags |= NBD_CMD_FLAG_NO_HOLE;
     }
-    if (request->type != NBD_CMD_WRITE_ZEROES &&
-        (request->flags & NBD_CMD_FLAG_NO_HOLE)) {
-        error_setg(errp, "unexpected flags (got 0x%x)", request->flags);
+    if (request->flags & ~valid_flags) {
+        error_setg(errp, "unsupported flags for command %s (got 0x%x)",
+                   nbd_cmd_lookup(request->type), request->flags);
         return -EINVAL;
     }

@@ -1450,10 +1525,21 @@ reply:
         local_err = NULL;
     }

-    if (nbd_co_send_simple_reply(req->client, request.handle,
-                                 ret < 0 ? -ret : 0,
-                                 req->data, reply_data_len, &local_err) < 0)
-    {
+    if (client->structured_reply && request.type == NBD_CMD_READ) {
+        if (ret < 0) {
+            ret = nbd_co_send_structured_error(req->client, request.handle,
+                                               -ret, &local_err);
+        } else {
+            ret = nbd_co_send_structured_read(req->client, request.handle,
+                                              request.from, req->data,
+                                              reply_data_len, &local_err);
+        }
+    } else {
+        ret = nbd_co_send_simple_reply(req->client, request.handle,
+                                       ret < 0 ? -ret : 0,
+                                       req->data, reply_data_len, &local_err);
+    }
+    if (ret < 0) {
         error_prepend(&local_err, "Failed to send reply: ");
         goto disconnect;
     }
diff --git a/nbd/trace-events b/nbd/trace-events
index d3b702dd9a..15a9294445 100644
--- a/nbd/trace-events
+++ b/nbd/trace-events
@@ -56,6 +56,8 @@ nbd_receive_request(uint32_t magic, uint16_t flags, uint16_t type, uint64_t from
 nbd_blk_aio_attached(const char *name, void *ctx) "Export %s: Attaching clients to AIO context %p\n"
 nbd_blk_aio_detach(const char *name, void *ctx) "Export %s: Detaching clients from AIO context %p\n"
 nbd_co_send_simple_reply(uint64_t handle, uint32_t error, const char *errname, int len) "Send simple reply: handle = %" PRIu64 ", error = %" PRIu32 " (%s), len = %d"
+nbd_co_send_structured_read(uint64_t handle, uint64_t offset, void *data, size_t size) "Send structured read data reply: handle = %" PRIu64 ", offset = %" PRIu64 ", data = %p, len = %zu"
+nbd_co_send_structured_error(uint64_t handle, int err) "Send structured error reply: handle = %" PRIu64 ", error = %d"
 nbd_co_receive_request_decode_type(uint64_t handle, uint16_t type, const char *name) "Decoding type: handle = %" PRIu64 ", type = %" PRIu16 " (%s)"
 nbd_co_receive_request_payload_received(uint64_t handle, uint32_t len) "Payload received: handle = %" PRIu64 ", len = %" PRIu32
 nbd_co_receive_request_cmd_write(uint32_t len) "Reading %" PRIu32 " byte(s)"
-- 
2.13.6

Re: [Qemu-devel] [PATCH v4 4/8] nbd: Minimal structured read for server

[Vladimir Sementsov-Ogievskiy]

15.10.2017 04:01, Eric Blake wrote:
> From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
> Minimal implementation of structured read: one structured reply chunk,
> no segmentation.
> Minimal structured error implementation: no text message.
> Support DF flag, but just ignore it, as there is no segmentation any
> way.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v4: better _DF flag handling, convert errno to wire format, add
> comments and tracing, rework structured error for less churn when adding
> text message later, don't kill connection on redundant client option
> ---
>   nbd/server.c     | 106 +++++++++++++++++++++++++++++++++++++++++++++++++------
>   nbd/trace-events |   2 ++
>   2 files changed, 98 insertions(+), 10 deletions(-)
>
> diff --git a/nbd/server.c b/nbd/server.c
> index efb6003364..23dc6be708 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -100,6 +100,8 @@ struct NBDClient {
>       QTAILQ_ENTRY(NBDClient) next;
>       int nb_requests;
>       bool closing;
> +
> +    bool structured_reply;
>   };
>
>   /* That's all folks */
> @@ -762,6 +764,23 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags,
>                       return ret;
>                   }
>                   break;
> +
> +            case NBD_OPT_STRUCTURED_REPLY:
> +                if (client->structured_reply) {
> +                    ret = nbd_negotiate_send_rep_err(
> +                        client->ioc, NBD_REP_ERR_INVALID, option, errp,
> +                        "structured reply already negotiated");

You were going to send a patch to spec for this..

> +                } else {
> +                    ret = nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK,
> +                                                 option, errp);
> +                }
> +                if (ret < 0) {
> +                    return ret;
> +                }
> +                client->structured_reply = true;
> +                myflags |= NBD_CMD_FLAG_DF;

it should be NBD_FLAG_SEND_DF

[...]

the following looks ok.

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v4 4/8] nbd: Minimal structured read for server

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2097 bytes --]

On 10/16/2017 04:49 AM, Vladimir Sementsov-Ogievskiy wrote:
> 15.10.2017 04:01, Eric Blake wrote:
>> From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>
>> Minimal implementation of structured read: one structured reply chunk,
>> no segmentation.
>> Minimal structured error implementation: no text message.
>> Support DF flag, but just ignore it, as there is no segmentation any
>> way.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>

>> +
>> +            case NBD_OPT_STRUCTURED_REPLY:
>> +                if (client->structured_reply) {
>> +                    ret = nbd_negotiate_send_rep_err(
>> +                        client->ioc, NBD_REP_ERR_INVALID, option, errp,
>> +                        "structured reply already negotiated");
> 
> You were going to send a patch to spec for this..

Still am; it's on my queue for tasks to do today. :)

> 
>> +                } else {
>> +                    ret = nbd_negotiate_send_rep(client->ioc,
>> NBD_REP_ACK,
>> +                                                 option, errp);
>> +                }
>> +                if (ret < 0) {
>> +                    return ret;
>> +                }
>> +                client->structured_reply = true;
>> +                myflags |= NBD_CMD_FLAG_DF;
> 
> it should be NBD_FLAG_SEND_DF

Ouch, you're right.  NBD_CMD_FLAG_DF has the same value as
NBD_FLAG_SEND_FLUSH (which we always advertise).  If nothing else, you
proved that my weekend hacking was shoved out the door quickly, rather
than fully tested.  :)

> 
> [...]
> 
> the following looks ok.
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v4 4/8] nbd: Minimal structured read for server

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1325 bytes --]

On 10/16/2017 07:18 AM, Eric Blake wrote:
> On 10/16/2017 04:49 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 15.10.2017 04:01, Eric Blake wrote:
>>> From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>
>>> Minimal implementation of structured read: one structured reply chunk,
>>> no segmentation.
>>> Minimal structured error implementation: no text message.
>>> Support DF flag, but just ignore it, as there is no segmentation any
>>> way.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>>
> 
>>> +
>>> +            case NBD_OPT_STRUCTURED_REPLY:
>>> +                if (client->structured_reply) {
>>> +                    ret = nbd_negotiate_send_rep_err(
>>> +                        client->ioc, NBD_REP_ERR_INVALID, option, errp,
>>> +                        "structured reply already negotiated");
>>
>> You were going to send a patch to spec for this..
> 
> Still am; it's on my queue for tasks to do today. :)

https://lists.debian.org/nbd/2017/nbd-201710/msg00013.html

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v4 4/8] nbd: Minimal structured read for server

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1695 bytes --]

On 10/14/2017 08:01 PM, Eric Blake wrote:
> From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> Minimal implementation of structured read: one structured reply chunk,
> no segmentation.
> Minimal structured error implementation: no text message.
> Support DF flag, but just ignore it, as there is no segmentation any
> way.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> 

> +
> +            case NBD_OPT_STRUCTURED_REPLY:
> +                if (client->structured_reply) {
> +                    ret = nbd_negotiate_send_rep_err(
> +                        client->ioc, NBD_REP_ERR_INVALID, option, errp,
> +                        "structured reply already negotiated");
> +                } else {
> +                    ret = nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK,
> +                                                 option, errp);
> +                }

Fails spectacularly if the client sent a non-zero length payload along
with the option, because we forgot to nbd_drop the payload, getting the
server out-of-sync with the client's next option request.  I'm
requesting clarification from the NBD list on how to best handle that
(presumably with NBD_REP_ERR_INVALID), and in the meantime will copy
what we do for NBD_OPT_LIST with unexpected payload.  It also made me
realize that we don't reject unexpected payload for NBD_OPT_STARTTLS; a
common helper function would make that easier, so guess what I'm adding
in v5 :)

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

[Qemu-devel] [PATCH v4 5/8] nbd/server: Include human-readable message in structured errors

[Eric Blake]

The NBD spec permits including a human-readable error string if
structured replies are in force, so we might as well send the
client the message that we logged on any error.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 nbd/server.c     | 22 ++++++++++++++++------
 nbd/trace-events |  2 +-
 2 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 23dc6be708..8085d79076 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1289,23 +1289,25 @@ static int coroutine_fn nbd_co_send_structured_read(NBDClient *client,
 static int coroutine_fn nbd_co_send_structured_error(NBDClient *client,
                                                      uint64_t handle,
                                                      uint32_t error,
+                                                     char *msg,
                                                      Error **errp)
 {
     NBDStructuredError chunk;
     int nbd_err = system_errno_to_nbd_errno(error);
     struct iovec iov[] = {
         {.iov_base = &chunk, .iov_len = sizeof(chunk)},
-        /* FIXME: Support human-readable error message */
+        {.iov_base = msg, .iov_len = strlen(msg)},
     };

     assert(nbd_err);
-    trace_nbd_co_send_structured_error(handle, nbd_err);
+    trace_nbd_co_send_structured_error(handle, nbd_err,
+                                       nbd_err_lookup(nbd_err), msg);
     set_be_chunk(&chunk.h, NBD_REPLY_FLAG_DONE, NBD_REPLY_TYPE_ERROR, handle,
                  sizeof(chunk) - sizeof(chunk.h));
     stl_be_p(&chunk.error, nbd_err);
-    stw_be_p(&chunk.message_length, 0);
+    stw_be_p(&chunk.message_length, iov[1].iov_len);

-    return nbd_co_send_iov(client, iov, 1, errp);
+    return nbd_co_send_iov(client, iov, 1 + !!iov[1].iov_len, errp);
 }

 /* nbd_co_receive_request
@@ -1406,6 +1408,7 @@ static coroutine_fn void nbd_trip(void *opaque)
     int flags;
     int reply_data_len = 0;
     Error *local_err = NULL;
+    char *msg = NULL;

     trace_nbd_trip();
     if (client->closing) {
@@ -1521,14 +1524,20 @@ reply:
     if (local_err) {
         /* If we get here, local_err was not a fatal error, and should be sent
          * to the client. */
+        assert(ret < 0);
+        msg = g_strdup(error_get_pretty(local_err));
         error_report_err(local_err);
         local_err = NULL;
     }

-    if (client->structured_reply && request.type == NBD_CMD_READ) {
+    if (client->structured_reply &&
+        (ret < 0 || request.type == NBD_CMD_READ)) {
         if (ret < 0) {
+            if (!msg) {
+                msg = g_strdup("");
+            }
             ret = nbd_co_send_structured_error(req->client, request.handle,
-                                               -ret, &local_err);
+                                               -ret, msg, &local_err);
         } else {
             ret = nbd_co_send_structured_read(req->client, request.handle,
                                               request.from, req->data,
@@ -1539,6 +1548,7 @@ reply:
                                        ret < 0 ? -ret : 0,
                                        req->data, reply_data_len, &local_err);
     }
+    g_free(msg);
     if (ret < 0) {
         error_prepend(&local_err, "Failed to send reply: ");
         goto disconnect;
diff --git a/nbd/trace-events b/nbd/trace-events
index 15a9294445..880f211c07 100644
--- a/nbd/trace-events
+++ b/nbd/trace-events
@@ -57,7 +57,7 @@ nbd_blk_aio_attached(const char *name, void *ctx) "Export %s: Attaching clients
 nbd_blk_aio_detach(const char *name, void *ctx) "Export %s: Detaching clients from AIO context %p\n"
 nbd_co_send_simple_reply(uint64_t handle, uint32_t error, const char *errname, int len) "Send simple reply: handle = %" PRIu64 ", error = %" PRIu32 " (%s), len = %d"
 nbd_co_send_structured_read(uint64_t handle, uint64_t offset, void *data, size_t size) "Send structured read data reply: handle = %" PRIu64 ", offset = %" PRIu64 ", data = %p, len = %zu"
-nbd_co_send_structured_error(uint64_t handle, int err) "Send structured error reply: handle = %" PRIu64 ", error = %d"
+nbd_co_send_structured_error(uint64_t handle, int err, const char *errname, const char *msg) "Send structured error reply: handle = %" PRIu64 ", error = %d (%s), msg = '%s'"
 nbd_co_receive_request_decode_type(uint64_t handle, uint16_t type, const char *name) "Decoding type: handle = %" PRIu64 ", type = %" PRIu16 " (%s)"
 nbd_co_receive_request_payload_received(uint64_t handle, uint32_t len) "Payload received: handle = %" PRIu64 ", len = %" PRIu32
 nbd_co_receive_request_cmd_write(uint32_t len) "Reading %" PRIu32 " byte(s)"
-- 
2.13.6

Re: [Qemu-devel] [PATCH v4 5/8] nbd/server: Include human-readable message in structured errors

[Vladimir Sementsov-Ogievskiy]

15.10.2017 04:01, Eric Blake wrote:
> The NBD spec permits including a human-readable error string if
> structured replies are in force, so we might as well send the
> client the message that we logged on any error.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>   nbd/server.c     | 22 ++++++++++++++++------
>   nbd/trace-events |  2 +-
>   2 files changed, 17 insertions(+), 7 deletions(-)
>
> diff --git a/nbd/server.c b/nbd/server.c
> index 23dc6be708..8085d79076 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -1289,23 +1289,25 @@ static int coroutine_fn nbd_co_send_structured_read(NBDClient *client,
>   static int coroutine_fn nbd_co_send_structured_error(NBDClient *client,
>                                                        uint64_t handle,
>                                                        uint32_t error,
> +                                                     char *msg,

it's not const because we want to put it into iov..
may be it is better to make it const and convert to (char *) like in 
qio_channel_write_all, to
1: more native message parameter type
2: allow constat strings like nbd_co_send_structured_error(..., "some 
error")

>                                                        Error **errp)
>   {
>       NBDStructuredError chunk;
>       int nbd_err = system_errno_to_nbd_errno(error);
>       struct iovec iov[] = {
>           {.iov_base = &chunk, .iov_len = sizeof(chunk)},
> -        /* FIXME: Support human-readable error message */
> +        {.iov_base = msg, .iov_len = strlen(msg)},
>       };

msg is always non-zero? so we don't want to send errors without 
message.. (1)

>
>       assert(nbd_err);
> -    trace_nbd_co_send_structured_error(handle, nbd_err);
> +    trace_nbd_co_send_structured_error(handle, nbd_err,
> +                                       nbd_err_lookup(nbd_err), msg);

it doesn't really matter, but "nbd_err_lookup(nbd_err)" is a bit 
unrelated and looks like part of previous patch

>       set_be_chunk(&chunk.h, NBD_REPLY_FLAG_DONE, NBD_REPLY_TYPE_ERROR, handle,
>                    sizeof(chunk) - sizeof(chunk.h));
>       stl_be_p(&chunk.error, nbd_err);
> -    stw_be_p(&chunk.message_length, 0);
> +    stw_be_p(&chunk.message_length, iov[1].iov_len);
>
> -    return nbd_co_send_iov(client, iov, 1, errp);
> +    return nbd_co_send_iov(client, iov, 1 + !!iov[1].iov_len, errp);

but you allow messages of zero length..
it's ok, but looks a bit strange in connection with (1)

>   }
>
>   /* nbd_co_receive_request
> @@ -1406,6 +1408,7 @@ static coroutine_fn void nbd_trip(void *opaque)
>       int flags;
>       int reply_data_len = 0;
>       Error *local_err = NULL;
> +    char *msg = NULL;
>
>       trace_nbd_trip();
>       if (client->closing) {
> @@ -1521,14 +1524,20 @@ reply:
>       if (local_err) {
>           /* If we get here, local_err was not a fatal error, and should be sent
>            * to the client. */
> +        assert(ret < 0);
> +        msg = g_strdup(error_get_pretty(local_err));
>           error_report_err(local_err);
>           local_err = NULL;
>       }
>
> -    if (client->structured_reply && request.type == NBD_CMD_READ) {
> +    if (client->structured_reply &&
> +        (ret < 0 || request.type == NBD_CMD_READ)) {
>           if (ret < 0) {
> +            if (!msg) {
> +                msg = g_strdup("");

I'd prefer to handle msg=NULL in nbd_co_send_structured_error instead of 
this.

> +            }
>               ret = nbd_co_send_structured_error(req->client, request.handle,
> -                                               -ret, &local_err);
> +                                               -ret, msg, &local_err);
>           } else {
>               ret = nbd_co_send_structured_read(req->client, request.handle,
>                                                 request.from, req->data,
> @@ -1539,6 +1548,7 @@ reply:
>                                          ret < 0 ? -ret : 0,
>                                          req->data, reply_data_len, &local_err);
>       }
> +    g_free(msg);
>       if (ret < 0) {
>           error_prepend(&local_err, "Failed to send reply: ");
>           goto disconnect;
> diff --git a/nbd/trace-events b/nbd/trace-events
> index 15a9294445..880f211c07 100644
> --- a/nbd/trace-events
> +++ b/nbd/trace-events
> @@ -57,7 +57,7 @@ nbd_blk_aio_attached(const char *name, void *ctx) "Export %s: Attaching clients
>   nbd_blk_aio_detach(const char *name, void *ctx) "Export %s: Detaching clients from AIO context %p\n"
>   nbd_co_send_simple_reply(uint64_t handle, uint32_t error, const char *errname, int len) "Send simple reply: handle = %" PRIu64 ", error = %" PRIu32 " (%s), len = %d"
>   nbd_co_send_structured_read(uint64_t handle, uint64_t offset, void *data, size_t size) "Send structured read data reply: handle = %" PRIu64 ", offset = %" PRIu64 ", data = %p, len = %zu"
> -nbd_co_send_structured_error(uint64_t handle, int err) "Send structured error reply: handle = %" PRIu64 ", error = %d"
> +nbd_co_send_structured_error(uint64_t handle, int err, const char *errname, const char *msg) "Send structured error reply: handle = %" PRIu64 ", error = %d (%s), msg = '%s'"
>   nbd_co_receive_request_decode_type(uint64_t handle, uint16_t type, const char *name) "Decoding type: handle = %" PRIu64 ", type = %" PRIu16 " (%s)"
>   nbd_co_receive_request_payload_received(uint64_t handle, uint32_t len) "Payload received: handle = %" PRIu64 ", len = %" PRIu32
>   nbd_co_receive_request_cmd_write(uint32_t len) "Reading %" PRIu32 " byte(s)"

anyway it should work, so, with or without my suggestions:

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

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v4 5/8] nbd/server: Include human-readable message in structured errors

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 5050 bytes --]

On 10/16/2017 05:59 AM, Vladimir Sementsov-Ogievskiy wrote:
> 15.10.2017 04:01, Eric Blake wrote:
>> The NBD spec permits including a human-readable error string if
>> structured replies are in force, so we might as well send the
>> client the message that we logged on any error.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>>   nbd/server.c     | 22 ++++++++++++++++------
>>   nbd/trace-events |  2 +-
>>   2 files changed, 17 insertions(+), 7 deletions(-)
>>
>> diff --git a/nbd/server.c b/nbd/server.c
>> index 23dc6be708..8085d79076 100644
>> --- a/nbd/server.c
>> +++ b/nbd/server.c
>> @@ -1289,23 +1289,25 @@ static int coroutine_fn
>> nbd_co_send_structured_read(NBDClient *client,
>>   static int coroutine_fn nbd_co_send_structured_error(NBDClient *client,
>>                                                        uint64_t handle,
>>                                                        uint32_t error,
>> +                                                     char *msg,
> 
> it's not const because we want to put it into iov..
> may be it is better to make it const and convert to (char *) like in
> qio_channel_write_all, to
> 1: more native message parameter type
> 2: allow constat strings like nbd_co_send_structured_error(..., "some
> error")

Yes, casting away const would be a more convenient interface (it's a
bummer that iov can't be const-correct for write, because it is also
used by read which cannot be const).

> 
>>                                                        Error **errp)
>>   {
>>       NBDStructuredError chunk;
>>       int nbd_err = system_errno_to_nbd_errno(error);
>>       struct iovec iov[] = {
>>           {.iov_base = &chunk, .iov_len = sizeof(chunk)},
>> -        /* FIXME: Support human-readable error message */
>> +        {.iov_base = msg, .iov_len = strlen(msg)},
>>       };
> 
> msg is always non-zero? so we don't want to send errors without
> message.. (1)

I played with the idea of allowing NULL, and don't know whether it was
easier to require non-NULL message (which may require NULL check at all
callers) or to allow NULL (requiring more code here).

> 
>>
>>       assert(nbd_err);
>> -    trace_nbd_co_send_structured_error(handle, nbd_err);
>> +    trace_nbd_co_send_structured_error(handle, nbd_err,
>> +                                       nbd_err_lookup(nbd_err), msg);
> 
> it doesn't really matter, but "nbd_err_lookup(nbd_err)" is a bit
> unrelated and looks like part of previous patch

The previous patch was yours; I tried to minimize the changes I made to
your patches, so I stuck this one in mine instead. But if you think we
should trace accurately from the get-go, I'm just fine rebasing the
series to make your patch trace the error name at its introduction,
rather than in my followup that supports error messages.

> 
>>       set_be_chunk(&chunk.h, NBD_REPLY_FLAG_DONE,
>> NBD_REPLY_TYPE_ERROR, handle,
>>                    sizeof(chunk) - sizeof(chunk.h));
>>       stl_be_p(&chunk.error, nbd_err);
>> -    stw_be_p(&chunk.message_length, 0);
>> +    stw_be_p(&chunk.message_length, iov[1].iov_len);
>>
>> -    return nbd_co_send_iov(client, iov, 1, errp);
>> +    return nbd_co_send_iov(client, iov, 1 + !!iov[1].iov_len, errp);
> 
> but you allow messages of zero length..
> it's ok, but looks a bit strange in connection with (1)

Passing "" is the easiest thing to do for callers that can't pass NULL.
My first attempt had:

.iov_len = msg ? strlen(msg) : 0

and

trace_...(msg ? msg : "")

For this patch, there is only a single caller, so it's really hard to
judge which style (required non-NULL parameter, vs. doing more work in
the common function) will be more useful until we add further callers.


>> -    if (client->structured_reply && request.type == NBD_CMD_READ) {
>> +    if (client->structured_reply &&
>> +        (ret < 0 || request.type == NBD_CMD_READ)) {
>>           if (ret < 0) {
>> +            if (!msg) {
>> +                msg = g_strdup("");
> 
> I'd prefer to handle msg=NULL in nbd_co_send_structured_error instead of
> this.

Okay, then it sounds like accepting a NULL parameter is the way to go!

It's also worth considering that casting away const would mean I don't
have to g_strdup("").


> anyway it should work, so, with or without my suggestions:
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Thanks for your reviews!

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v4 5/8] nbd/server: Include human-readable message in structured errors

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2581 bytes --]

On 10/14/2017 08:01 PM, Eric Blake wrote:
> The NBD spec permits including a human-readable error string if
> structured replies are in force, so we might as well send the
> client the message that we logged on any error.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  nbd/server.c     | 22 ++++++++++++++++------
>  nbd/trace-events |  2 +-
>  2 files changed, 17 insertions(+), 7 deletions(-)
> 

>      assert(nbd_err);
> -    trace_nbd_co_send_structured_error(handle, nbd_err);
> +    trace_nbd_co_send_structured_error(handle, nbd_err,
> +                                       nbd_err_lookup(nbd_err), msg);
>      set_be_chunk(&chunk.h, NBD_REPLY_FLAG_DONE, NBD_REPLY_TYPE_ERROR, handle,
>                   sizeof(chunk) - sizeof(chunk.h));

Bug - it's a bad idea to not include the message length in the overall
length, because the client then gets out of sync with the server (it
reads only 6 bytes instead of 6+strlen(msg) bytes, and expects the
message to start with the magic number for the next reply).

>      stl_be_p(&chunk.error, nbd_err);
> -    stw_be_p(&chunk.message_length, 0);
> +    stw_be_p(&chunk.message_length, iov[1].iov_len);

But this also highlights a bug in 9/8, where we have:

> +static int nbd_parse_error_payload(NBDStructuredReplyChunk *chunk,
> +                                   uint8_t *payload, int *request_ret,
> +                                   Error **errp)
> +{
> +    uint32_t error;
> +    uint16_t message_size;
> +
> +    assert(chunk->type & (1 << 15));
> +
> +    if (chunk->length < sizeof(error) + sizeof(message_size)) {
> +        error_setg(errp,
> +                   "Protocol error: invalid payload for structured error");
> +        return -EINVAL;
> +    }
> +
> +    error = nbd_errno_to_system_errno(payload_advance32(&payload));
> +    if (error == 0) {
> +        error_setg(errp, "Protocol error: server sent structured error chunk"
> +                         "with error = 0");
> +        return -EINVAL;
> +    }
> +
> +    *request_ret = error;
> +    message_size = payload_advance16(&payload);
> +    error_setg_errno(errp, error, "%.*s", message_size, payload);

Whoops - no sanity check that message_size fits within chunk->length.
So when we read message_length 33 (when the server sends a message 33
bytes long), we are then dereferencing up to 33 bytes of garbage beyond
the end of payload.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

[Qemu-devel] [PATCH v4 6/8] nbd/client: refactor nbd_receive_starttls

[Eric Blake]

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

Split out nbd_request_simple_option to be reused for structured reply
option.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Eric Blake <eblake@redhat.com>

---
v4: reduce redundant traces, typo fix in commit message
---
 nbd/client.c     | 69 ++++++++++++++++++++++++++++++++++++++------------------
 nbd/trace-events |  4 +---
 2 files changed, 48 insertions(+), 25 deletions(-)

diff --git a/nbd/client.c b/nbd/client.c
index 50f36b511e..9d335fcaba 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -508,35 +508,60 @@ static int nbd_receive_query_exports(QIOChannel *ioc,
     }
 }

+/* nbd_request_simple_option: Send an option request, and parse the reply
+ * return 1 for successful negotiation,
+ *        0 if operation is unsupported,
+ *        -1 with errp set for any other error
+ */
+static int nbd_request_simple_option(QIOChannel *ioc, int opt, Error **errp)
+{
+    nbd_opt_reply reply;
+
+    if (nbd_send_option_request(ioc, opt, 0, NULL, errp) < 0) {
+        return -1;
+    }
+
+    if (nbd_receive_option_reply(ioc, opt, &reply, errp) < 0) {
+        return -1;
+    }
+
+    if (reply.length != 0) {
+        error_setg(errp, "Option %d ('%s') response length is %" PRIu32
+                   " (it should be zero)", opt, nbd_opt_lookup(opt),
+                   reply.length);
+        nbd_send_opt_abort(ioc);
+        return -1;
+    }
+
+    if (reply.type == NBD_REP_ERR_UNSUP) {
+        return 0;
+    }
+
+    if (reply.type != NBD_REP_ACK) {
+        error_setg(errp, "Server rejected request for option %d (%s) "
+                   "with reply %" PRIx32 " (%s)", opt, nbd_opt_lookup(opt),
+                   reply.type, nbd_rep_lookup(reply.type));
+        nbd_send_opt_abort(ioc);
+        return -1;
+    }
+
+    return 1;
+}
+
 static QIOChannel *nbd_receive_starttls(QIOChannel *ioc,
                                         QCryptoTLSCreds *tlscreds,
                                         const char *hostname, Error **errp)
 {
-    nbd_opt_reply reply;
+    int ret;
     QIOChannelTLS *tioc;
     struct NBDTLSHandshakeData data = { 0 };

-    trace_nbd_receive_starttls_request();
-    if (nbd_send_option_request(ioc, NBD_OPT_STARTTLS, 0, NULL, errp) < 0) {
-        return NULL;
-    }
-
-    trace_nbd_receive_starttls_reply();
-    if (nbd_receive_option_reply(ioc, NBD_OPT_STARTTLS, &reply, errp) < 0) {
-        return NULL;
-    }
-
-    if (reply.type != NBD_REP_ACK) {
-        error_setg(errp, "Server rejected request to start TLS %" PRIx32,
-                   reply.type);
-        nbd_send_opt_abort(ioc);
-        return NULL;
-    }
-
-    if (reply.length != 0) {
-        error_setg(errp, "Start TLS response was not zero %" PRIu32,
-                   reply.length);
-        nbd_send_opt_abort(ioc);
+    ret = nbd_request_simple_option(ioc, NBD_OPT_STARTTLS, errp);
+    if (ret <= 0) {
+        if (ret == 0) {
+            error_setg(errp, "Server don't support STARTTLS option");
+            nbd_send_opt_abort(ioc);
+        }
         return NULL;
     }

diff --git a/nbd/trace-events b/nbd/trace-events
index 880f211c07..39b723d1d8 100644
--- a/nbd/trace-events
+++ b/nbd/trace-events
@@ -8,9 +8,7 @@ nbd_opt_go_info_unknown(int info, const char *name) "Ignoring unknown info %d (%
 nbd_opt_go_info_block_size(uint32_t minimum, uint32_t preferred, uint32_t maximum) "Block sizes are 0x%" PRIx32 ", 0x%" PRIx32 ", 0x%" PRIx32
 nbd_receive_query_exports_start(const char *wantname) "Querying export list for '%s'"
 nbd_receive_query_exports_success(const char *wantname) "Found desired export name '%s'"
-nbd_receive_starttls_request(void) "Requesting TLS from server"
-nbd_receive_starttls_reply(void) "Getting TLS reply from server"
-nbd_receive_starttls_new_client(void) "TLS request approved, setting up TLS"
+nbd_receive_starttls_new_client(void) "Setting up TLS"
 nbd_receive_starttls_tls_handshake(void) "Starting TLS handshake"
 nbd_receive_negotiate(void *tlscreds, const char *hostname) "Receiving negotiation tlscreds=%p hostname=%s"
 nbd_receive_negotiate_magic(uint64_t magic) "Magic is 0x%" PRIx64
-- 
2.13.6

Re: [Qemu-devel] [PATCH v4 6/8] nbd/client: refactor nbd_receive_starttls

[Vladimir Sementsov-Ogievskiy]

15.10.2017 04:01, Eric Blake wrote:
> From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
> Split out nbd_request_simple_option to be reused for structured reply
> option.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>

ok for me.


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v4 6/8] nbd/client: refactor nbd_receive_starttls

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1806 bytes --]

On 10/14/2017 08:01 PM, Eric Blake wrote:
> From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> Split out nbd_request_simple_option to be reused for structured reply
> option.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> 

> +static int nbd_request_simple_option(QIOChannel *ioc, int opt, Error **errp)
> +{
> +    nbd_opt_reply reply;
> +
> +    if (nbd_send_option_request(ioc, opt, 0, NULL, errp) < 0) {
> +        return -1;
> +    }
> +
> +    if (nbd_receive_option_reply(ioc, opt, &reply, errp) < 0) {
> +        return -1;
> +    }
> +
> +    if (reply.length != 0) {
> +        error_setg(errp, "Option %d ('%s') response length is %" PRIu32
> +                   " (it should be zero)", opt, nbd_opt_lookup(opt),
> +                   reply.length);
> +        nbd_send_opt_abort(ioc);
> +        return -1;
> +    }
> +
> +    if (reply.type == NBD_REP_ERR_UNSUP) {
> +        return 0;
> +    }

Oops, these two conditions are swapped.  A non-zero reply length is
perfectly acceptable if the server is sending NBD_REP_ERR_UNSUP with an
error message (as is the case with old qemu server, new qemu client).
We can only enforce non-zero length...

> +
> +    if (reply.type != NBD_REP_ACK) {
> +        error_setg(errp, "Server rejected request for option %d (%s) "
> +                   "with reply %" PRIx32 " (%s)", opt, nbd_opt_lookup(opt),
> +                   reply.type, nbd_rep_lookup(reply.type));
> +        nbd_send_opt_abort(ioc);
> +        return -1;
> +    }
> +

...here, after we know we got an ACK.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

[Qemu-devel] [PATCH v4 7/8] nbd/client: prepare nbd_receive_reply for structured reply

[Eric Blake]

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

In following patch nbd_receive_reply will be used both for simple
and structured reply header receiving.
NBDReply is altered into union of simple reply header and structured
reply chunk header, simple error translation moved to block/nbd-client
to be consistent with further structured reply error translation.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 include/block/nbd.h |  30 ++++++++++++---
 block/nbd-client.c  |   8 ++--
 nbd/client.c        | 104 +++++++++++++++++++++++++++++++++++++++++-----------
 nbd/trace-events    |   3 +-
 4 files changed, 113 insertions(+), 32 deletions(-)

diff --git a/include/block/nbd.h b/include/block/nbd.h
index 225e9575e4..2ee1578420 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -57,12 +57,6 @@ struct NBDRequest {
 };
 typedef struct NBDRequest NBDRequest;

-struct NBDReply {
-    uint64_t handle;
-    uint32_t error;
-};
-typedef struct NBDReply NBDReply;
-
 typedef struct NBDSimpleReply {
     uint32_t magic;  /* NBD_SIMPLE_REPLY_MAGIC */
     uint32_t error;
@@ -78,6 +72,20 @@ typedef struct NBDStructuredReplyChunk {
     uint32_t length; /* length of payload */
 } QEMU_PACKED NBDStructuredReplyChunk;

+typedef union NBDReply {
+    NBDSimpleReply simple;
+    NBDStructuredReplyChunk structured;
+    struct {
+        /* @magic and @handle fields have the same offset and size both in
+         * simple reply and structured reply chunk, so let them be accessible
+         * without ".simple." or ".structured." specification
+         */
+        uint32_t magic;
+        uint32_t _skip;
+        uint64_t handle;
+    } QEMU_PACKED;
+} NBDReply;
+
 /* Header of NBD_REPLY_TYPE_OFFSET_DATA, complete NBD_REPLY_TYPE_OFFSET_HOLE */
 typedef struct NBDStructuredRead {
     NBDStructuredReplyChunk h;
@@ -256,4 +264,14 @@ void nbd_client_put(NBDClient *client);
 void nbd_server_start(SocketAddress *addr, const char *tls_creds,
                       Error **errp);

+static inline bool nbd_reply_is_simple(NBDReply *reply)
+{
+    return reply->magic == NBD_SIMPLE_REPLY_MAGIC;
+}
+
+static inline bool nbd_reply_is_structured(NBDReply *reply)
+{
+    return reply->magic == NBD_STRUCTURED_REPLY_MAGIC;
+}
+
 #endif
diff --git a/block/nbd-client.c b/block/nbd-client.c
index c0683c3c83..58493b7ac4 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -92,7 +92,9 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)
         i = HANDLE_TO_INDEX(s, s->reply.handle);
         if (i >= MAX_NBD_REQUESTS ||
             !s->requests[i].coroutine ||
-            !s->requests[i].receiving) {
+            !s->requests[i].receiving ||
+            nbd_reply_is_structured(&s->reply))
+        {
             break;
         }

@@ -194,8 +196,8 @@ static int nbd_co_receive_reply(NBDClientSession *s,
         ret = -EIO;
     } else {
         assert(s->reply.handle == handle);
-        ret = -s->reply.error;
-        if (qiov && s->reply.error == 0) {
+        ret = -nbd_errno_to_system_errno(s->reply.simple.error);
+        if (qiov && ret == 0) {
             if (qio_channel_readv_all(s->ioc, qiov->iov, qiov->niov,
                                       NULL) < 0) {
                 ret = -EIO;
diff --git a/nbd/client.c b/nbd/client.c
index 9d335fcaba..eeb23ae934 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -907,6 +907,57 @@ int nbd_send_request(QIOChannel *ioc, NBDRequest *request)
     return nbd_write(ioc, buf, sizeof(buf), NULL);
 }

+/* nbd_receive_simple_reply
+ * Read simple reply except magic field (which should be already read).
+ * Payload is not read (payload is possible for CMD_READ, but here we even
+ * don't know whether it take place or not).
+ */
+static int nbd_receive_simple_reply(QIOChannel *ioc, NBDSimpleReply *reply,
+                                    Error **errp)
+{
+    int ret;
+
+    assert(reply->magic == NBD_SIMPLE_REPLY_MAGIC);
+
+    ret = nbd_read(ioc, (uint8_t *)reply + sizeof(reply->magic),
+                   sizeof(*reply) - sizeof(reply->magic), errp);
+    if (ret < 0) {
+        return ret;
+    }
+
+    be32_to_cpus(&reply->error);
+    be64_to_cpus(&reply->handle);
+
+    return 0;
+}
+
+/* nbd_receive_structured_reply_chunk
+ * Read structured reply chunk except magic field (which should be already
+ * read).
+ * Payload is not read.
+ */
+static int nbd_receive_structured_reply_chunk(QIOChannel *ioc,
+                                              NBDStructuredReplyChunk *chunk,
+                                              Error **errp)
+{
+    int ret;
+
+    assert(chunk->magic == NBD_STRUCTURED_REPLY_MAGIC);
+
+    ret = nbd_read(ioc, (uint8_t *)chunk + sizeof(chunk->magic),
+                   sizeof(*chunk) - sizeof(chunk->magic), errp);
+    if (ret < 0) {
+        return ret;
+    }
+
+    be16_to_cpus(&chunk->flags);
+    be16_to_cpus(&chunk->type);
+    be64_to_cpus(&chunk->handle);
+    be32_to_cpus(&chunk->length);
+
+    return 0;
+}
+
 /* nbd_receive_reply
  * Returns 1 on success
  *         0 on eof, when no data was read (errp is not set)
@@ -914,38 +965,47 @@ int nbd_send_request(QIOChannel *ioc, NBDRequest *request)
  */
 int nbd_receive_reply(QIOChannel *ioc, NBDReply *reply, Error **errp)
 {
-    uint8_t buf[NBD_REPLY_SIZE];
-    uint32_t magic;
     int ret;

-    ret = nbd_read_eof(ioc, buf, sizeof(buf), errp);
+    ret = nbd_read_eof(ioc, &reply->magic, sizeof(reply->magic), errp);
     if (ret <= 0) {
         return ret;
     }

-    /* Reply
-       [ 0 ..  3]    magic   (NBD_SIMPLE_REPLY_MAGIC)
-       [ 4 ..  7]    error   (0 == no error)
-       [ 7 .. 15]    handle
-     */
+    be32_to_cpus(&reply->magic);

-    magic = ldl_be_p(buf);
-    reply->error  = ldl_be_p(buf + 4);
-    reply->handle = ldq_be_p(buf + 8);
+    switch (reply->magic) {
+    case NBD_SIMPLE_REPLY_MAGIC:
+        ret = nbd_receive_simple_reply(ioc, &reply->simple, errp);
+        if (ret < 0) {
+            break;
+        }

-    trace_nbd_receive_reply(magic, reply->error, nbd_err_lookup(reply->error),
-                            reply->handle);
-    reply->error = nbd_errno_to_system_errno(reply->error);
-
-    if (reply->error == ESHUTDOWN) {
-        /* This works even on mingw which lacks a native ESHUTDOWN */
-        error_setg(errp, "server shutting down");
+        trace_nbd_receive_simple_reply(reply->simple.error,
+                                       nbd_err_lookup(reply->simple.error),
+                                       reply->handle);
+        if (reply->simple.error == NBD_ESHUTDOWN) {
+            /* This works even on mingw which lacks a native ESHUTDOWN */
+            error_setg(errp, "server shutting down");
+            return -EINVAL;
+        }
+        break;
+    case NBD_STRUCTURED_REPLY_MAGIC:
+        ret = nbd_receive_structured_reply_chunk(ioc, &reply->structured, errp);
+        if (ret < 0) {
+            break;
+        }
+        trace_nbd_receive_structured_reply_chunk(reply->structured.flags,
+                                                 reply->structured.type,
+                                                 reply->structured.handle,
+                                                 reply->structured.length);
+        break;
+    default:
+        error_setg(errp, "invalid magic (got 0x%" PRIx32 ")", reply->magic);
         return -EINVAL;
     }
-
-    if (magic != NBD_SIMPLE_REPLY_MAGIC) {
-        error_setg(errp, "invalid magic (got 0x%" PRIx32 ")", magic);
-        return -EINVAL;
+    if (ret < 0) {
+        return ret;
     }

     return 1;
diff --git a/nbd/trace-events b/nbd/trace-events
index 39b723d1d8..8af453467b 100644
--- a/nbd/trace-events
+++ b/nbd/trace-events
@@ -26,7 +26,8 @@ nbd_client_loop_ret(int ret, const char *error) "NBD loop returned %d: %s"
 nbd_client_clear_queue(void) "Clearing NBD queue"
 nbd_client_clear_socket(void) "Clearing NBD socket"
 nbd_send_request(uint64_t from, uint32_t len, uint64_t handle, uint16_t flags, uint16_t type, const char *name) "Sending request to server: { .from = %" PRIu64", .len = %" PRIu32 ", .handle = %" PRIu64 ", .flags = 0x%" PRIx16 ", .type = %" PRIu16 " (%s) }"
-nbd_receive_reply(uint32_t magic, int32_t error, const char *errname, uint64_t handle) "Got reply: { magic = 0x%" PRIx32 ", .error = % " PRId32 " (%s), handle = %" PRIu64" }"
+nbd_receive_simple_reply(int32_t error, const char *errname, uint64_t handle) "Got simple reply: { .error = % " PRId32 " (%s), handle = %" PRIu64" }"
+nbd_receive_structured_reply_chunk(uint16_t flags, uint16_t type, uint64_t handle, uint32_t length) "Got structured reply chunk: { flags = 0x%" PRIx16 ", type = %d, handle = %" PRIu64 ", length = %" PRIu32 " }"

 # nbd/common.c
 nbd_unknown_error(int err) "Squashing unexpected error %d to EINVAL"
-- 
2.13.6

Re: [Qemu-devel] [PATCH v4 7/8] nbd/client: prepare nbd_receive_reply for structured reply

[Vladimir Sementsov-Ogievskiy]

15.10.2017 04:01, Eric Blake wrote:
> From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
> In following patch nbd_receive_reply will be used both for simple
> and structured reply header receiving.
> NBDReply is altered into union of simple reply header and structured
> reply chunk header, simple error translation moved to block/nbd-client
> to be consistent with further structured reply error translation.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>

looks ok.



-- 
Best regards,
Vladimir

[Qemu-devel] [PATCH v4 8/8] nbd: Move nbd_read() to common header

[Eric Blake]

An upcoming change to block/nbd-client.c will want to read the
tail of a structured reply chunk directly from the wire.  Move
this function to make it easier.

Based on a patch from Vladimir Sementsov-Ogievskiy.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 include/block/nbd.h | 10 ++++++++++
 nbd/nbd-internal.h  |  9 ---------
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/include/block/nbd.h b/include/block/nbd.h
index 2ee1578420..da6e305dd5 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -264,6 +264,16 @@ void nbd_client_put(NBDClient *client);
 void nbd_server_start(SocketAddress *addr, const char *tls_creds,
                       Error **errp);

+
+/* nbd_read
+ * Reads @size bytes from @ioc. Returns 0 on success.
+ */
+static inline int nbd_read(QIOChannel *ioc, void *buffer, size_t size,
+                           Error **errp)
+{
+    return qio_channel_read_all(ioc, buffer, size, errp) < 0 ? -EIO : 0;
+}
+
 static inline bool nbd_reply_is_simple(NBDReply *reply)
 {
     return reply->magic == NBD_SIMPLE_REPLY_MAGIC;
diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h
index 4f24d6e57d..b64eb1cc9b 100644
--- a/nbd/nbd-internal.h
+++ b/nbd/nbd-internal.h
@@ -82,15 +82,6 @@ static inline int nbd_read_eof(QIOChannel *ioc, void *buffer, size_t size,
     return ret;
 }

-/* nbd_read
- * Reads @size bytes from @ioc. Returns 0 on success.
- */
-static inline int nbd_read(QIOChannel *ioc, void *buffer, size_t size,
-                           Error **errp)
-{
-    return qio_channel_read_all(ioc, buffer, size, errp) < 0 ? -EIO : 0;
-}
-
 /* nbd_write
  * Writes @size bytes to @ioc. Returns 0 on success.
  */
-- 
2.13.6

Re: [Qemu-devel] [PATCH v4 8/8] nbd: Move nbd_read() to common header

[Vladimir Sementsov-Ogievskiy]

15.10.2017 04:01, Eric Blake wrote:
> An upcoming change to block/nbd-client.c will want to read the
> tail of a structured reply chunk directly from the wire.  Move
> this function to make it easier.
>
> Based on a patch from Vladimir Sementsov-Ogievskiy.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>

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



-- 
Best regards,
Vladimir

[Qemu-devel] [PATCH v4 RFC 9/8] nbd: Minimal structured read for client

[Vladimir Sementsov-Ogievskiy]

Minimal implementation: for structured error only error_report error
message.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---

it's an updated
 [PATCH v3 13/13] nbd: Minimal structured read for client
 https://lists.gnu.org/archive/html/qemu-devel/2017-10/msg02760.html
from
 [PATCH v3 00/13] nbd minimal structured read
 https://lists.gnu.org/archive/html/qemu-devel/2017-10/msg02755.html
and now it is based on
 [PATCH v4 8/8] nbd: Move nbd_read() to common header
 https://lists.gnu.org/archive/html/qemu-devel/2017-10/msg03489.html
from
 [PATCH v4 0/8] nbd mimimal structured read
 https://lists.gnu.org/archive/html/qemu-devel/2017-10/msg03305.html

v4:
- add Error messages and errp to most of new functions
  (I try to return save first occurred error from structured reply,
   but rewrite it if fatal error occurred)
- add comment to NBDExportInfo
- move nbd_reply_type_lookup prot. to public .h (I think it should be
  merged into some previous patch of v4)
- remove ''#include "qemu/error-report.h"'
- rewrite payload_advance* helpers
- add comments about qemu_vfree for *payload
- _SREP_ -> _REPLY_
- move checking NBD_REPLY_FLAG_DONE for NBD_REPLY_TYPE_NONE
- do not negotiate structured reply from qemu-nbd client thread
  (make client->info.structured_reply in-out)

RFC: I don't like resulting error handling: it is rather confusing,
    requires a lot of definitions of "local_err" which looks bad. Propagating,
    setting error and then drop it for errp=NULL case - all it looks not
    very effective. Any ideas?
 include/block/nbd.h |  12 ++
 nbd/nbd-internal.h  |   1 -
 block/nbd-client.c  | 483 ++++++++++++++++++++++++++++++++++++++++++++++++----
 nbd/client.c        |  10 ++
 4 files changed, 474 insertions(+), 32 deletions(-)

diff --git a/include/block/nbd.h b/include/block/nbd.h
index da6e305dd5..92d1723d7c 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -197,6 +197,11 @@ enum {
 #define NBD_REPLY_TYPE_ERROR         NBD_REPLY_ERR(1)
 #define NBD_REPLY_TYPE_ERROR_OFFSET  NBD_REPLY_ERR(2)
 
+static inline bool nbd_reply_type_is_error(int type)
+{
+    return type & (1 << 15);
+}
+
 /* NBD errors are based on errno numbers, so there is a 1:1 mapping,
  * but only a limited set of errno values is specified in the protocol.
  * Everything else is squashed to EINVAL.
@@ -214,6 +219,11 @@ enum {
 struct NBDExportInfo {
     /* Set by client before nbd_receive_negotiate() */
     bool request_sizes;
+
+    /* In-out fields, set by client before nbd_receive_negotiate() and
+     * updated by server results during nbd_receive_negotiate() */
+    bool structured_reply;
+
     /* Set by server results during nbd_receive_negotiate() */
     uint64_t size;
     uint16_t flags;
@@ -284,4 +294,6 @@ static inline bool nbd_reply_is_structured(NBDReply *reply)
     return reply->magic == NBD_STRUCTURED_REPLY_MAGIC;
 }
 
+const char *nbd_reply_type_lookup(uint16_t type);
+
 #endif
diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h
index b64eb1cc9b..eeff78d3c9 100644
--- a/nbd/nbd-internal.h
+++ b/nbd/nbd-internal.h
@@ -104,7 +104,6 @@ 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_reply_type_lookup(uint16_t type);
 const char *nbd_err_lookup(int err);
 
 int nbd_drop(QIOChannel *ioc, size_t size, Error **errp);
diff --git a/block/nbd-client.c b/block/nbd-client.c
index 58493b7ac4..9d9e75643e 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -93,7 +93,7 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)
         if (i >= MAX_NBD_REQUESTS ||
             !s->requests[i].coroutine ||
             !s->requests[i].receiving ||
-            nbd_reply_is_structured(&s->reply))
+            (nbd_reply_is_structured(&s->reply) && !s->info.structured_reply))
         {
             break;
         }
@@ -181,75 +181,486 @@ err:
     return rc;
 }
 
-static int nbd_co_receive_reply(NBDClientSession *s,
-                                uint64_t handle,
-                                QEMUIOVector *qiov)
+static inline uint16_t payload_advance16(uint8_t **payload)
+{
+    *payload += 2;
+    return lduw_be_p(*payload - 2);
+}
+
+static inline uint32_t payload_advance32(uint8_t **payload)
+{
+    *payload += 4;
+    return lduw_be_p(*payload - 4);
+}
+
+static inline uint64_t payload_advance64(uint8_t **payload)
+{
+    *payload += 8;
+    return lduw_be_p(*payload - 8);
+}
+
+static int nbd_parse_offset_hole_payload(NBDStructuredReplyChunk *chunk,
+                                         uint8_t *payload, QEMUIOVector *qiov,
+                                         Error **errp)
+{
+    uint64_t offset;
+    uint32_t hole_size;
+
+    if (chunk->length != sizeof(offset) + sizeof(hole_size)) {
+        error_setg(errp, "Protocol error: invalid payload for "
+                         "NBD_REPLY_TYPE_OFFSET_HOLE");
+        return -EINVAL;
+    }
+
+    offset = payload_advance64(&payload);
+    hole_size = payload_advance32(&payload);
+
+    if (offset + hole_size > qiov->size) {
+        error_setg(errp, "Protocol error: server sent chunk exceeding requested"
+                         " region");
+        return -EINVAL;
+    }
+
+    qemu_iovec_memset(qiov, offset, 0, hole_size);
+
+    return 0;
+}
+
+/* nbd_parse_error_payload
+ * on success @errp contains message describing nbd error reply
+ */
+static int nbd_parse_error_payload(NBDStructuredReplyChunk *chunk,
+                                   uint8_t *payload, int *request_ret,
+                                   Error **errp)
+{
+    uint32_t error;
+    uint16_t message_size;
+
+    assert(chunk->type & (1 << 15));
+
+    if (chunk->length < sizeof(error) + sizeof(message_size)) {
+        error_setg(errp,
+                   "Protocol error: invalid payload for structured error");
+        return -EINVAL;
+    }
+
+    error = nbd_errno_to_system_errno(payload_advance32(&payload));
+    if (error == 0) {
+        error_setg(errp, "Protocol error: server sent structured error chunk"
+                         "with error = 0");
+        return -EINVAL;
+    }
+
+    *request_ret = error;
+    message_size = payload_advance16(&payload);
+    error_setg_errno(errp, error, "%.*s", message_size, payload);
+
+    /* TODO handle ERROR_OFFSET */
+
+    return 0;
+}
+
+static int nbd_co_receive_offset_data_payload(NBDClientSession *s,
+                                              QEMUIOVector *qiov, Error **errp)
+{
+    QEMUIOVector sub_qiov;
+    uint64_t offset;
+    size_t data_size;
+    int ret;
+    NBDStructuredReplyChunk *chunk = &s->reply.structured;
+
+    assert(nbd_reply_is_structured(&s->reply));
+
+    if (chunk->length < sizeof(offset)) {
+        error_setg(errp, "Protocol error: invalid payload for "
+                         "NBD_REPLY_TYPE_OFFSET_DATA");
+        return -EINVAL;
+    }
+
+    if (nbd_read(s->ioc, &offset, sizeof(offset), errp) < 0) {
+        return -EIO;
+    }
+    be64_to_cpus(&offset);
+
+    data_size = chunk->length - sizeof(offset);
+    if (offset + data_size > qiov->size) {
+        error_setg(errp, "Protocol error: server sent chunk exceeding requested"
+                         " region");
+        return -EINVAL;
+    }
+
+    qemu_iovec_init(&sub_qiov, qiov->niov);
+    qemu_iovec_concat(&sub_qiov, qiov, offset, data_size);
+    ret = qio_channel_readv_all(s->ioc, sub_qiov.iov, sub_qiov.niov, errp);
+    qemu_iovec_destroy(&sub_qiov);
+
+    return ret < 0 ? -EIO : 0;
+}
+
+#define NBD_MAX_MALLOC_PAYLOAD 1000
+/* nbd_co_receive_structured_payload
+ * The resulting pointer stored in @payload requires qemu_vfree() to free it.
+ */
+static coroutine_fn int nbd_co_receive_structured_payload(
+        NBDClientSession *s, void **payload, Error **errp)
+{
+    int ret;
+    uint32_t len;
+
+    assert(nbd_reply_is_structured(&s->reply));
+
+    len = s->reply.structured.length;
+
+    if (len == 0) {
+        return 0;
+    }
+
+    if (payload == NULL) {
+        error_setg(errp, "Unexpected structured payload");
+        return -EINVAL;
+    }
+
+    if (len > NBD_MAX_MALLOC_PAYLOAD) {
+        error_setg(errp, "Too large payload");
+        return -EINVAL;
+    }
+
+    *payload = qemu_memalign(8, len);
+    ret = nbd_read(s->ioc, *payload, len, errp);
+    if (ret < 0) {
+        qemu_vfree(*payload);
+        *payload = NULL;
+        return ret;
+    }
+
+    return 0;
+}
+
+/* nbd_co_do_receive_one_chunk
+ * for simple reply:
+ *   set request_ret to received reply error
+ *   if qiov is not NULL: read payload to @qiov
+ * for structured reply chunk:
+ *   if error chunk: read payload, set @request_ret, do not set @payload
+ *   else if offset_data chunk: read payload data to @qiov, do not set @payload
+ *   else: read payload to @payload
+ *
+ * The pointer stored in @payload requires qemu_vfree() to free it.
+ * If function fails @errp containts corresponding error message. If function
+ * doesn't fail but reply contains error @request_ret is set to corresponding
+ * return code and errp is set to corresponding error message.
+ */
+static coroutine_fn int nbd_co_do_receive_one_chunk(
+        NBDClientSession *s, uint64_t handle, bool only_structured,
+        int *request_ret, QEMUIOVector *qiov, void **payload, Error **errp)
 {
     int ret;
     int i = HANDLE_TO_INDEX(s, handle);
+    void *local_payload = NULL;
+    NBDStructuredReplyChunk *chunk;
+
+    if (payload) {
+        *payload = NULL;
+    }
+    *request_ret = 0;
 
     /* Wait until we're woken up by nbd_read_reply_entry.  */
     s->requests[i].receiving = true;
     qemu_coroutine_yield();
     s->requests[i].receiving = false;
     if (!s->ioc || s->quit) {
-        ret = -EIO;
-    } else {
-        assert(s->reply.handle == handle);
-        ret = -nbd_errno_to_system_errno(s->reply.simple.error);
-        if (qiov && ret == 0) {
-            if (qio_channel_readv_all(s->ioc, qiov->iov, qiov->niov,
-                                      NULL) < 0) {
-                ret = -EIO;
-                s->quit = true;
+        error_setg(errp, "Connection closed");
+        return -EIO;
+    }
+
+    assert(s->reply.handle == handle);
+
+    if (nbd_reply_is_simple(&s->reply)) {
+        if (only_structured) {
+            error_setg(errp, "Protocol error: simple reply when structured"
+                             "reply chunk was expected");
+            return -EINVAL;
+        }
+
+        *request_ret = -nbd_errno_to_system_errno(s->reply.simple.error);
+        if (*request_ret < 0 || !qiov) {
+            if (*request_ret < 0) {
+                error_setg_errno(errp, -*request_ret, "Request failed");
             }
+            return 0;
         }
 
-        /* Tell the read handler to read another header.  */
-        s->reply.handle = 0;
+        return qio_channel_readv_all(s->ioc, qiov->iov, qiov->niov,
+                                     errp) < 0 ? -EIO : 0;
     }
 
-    s->requests[i].coroutine = NULL;
+    /* handle structured reply chunk */
+    assert(s->info.structured_reply);
+    chunk = &s->reply.structured;
+
+    if (chunk->type == NBD_REPLY_TYPE_NONE) {
+        if (!(chunk->flags & NBD_REPLY_FLAG_DONE)) {
+            error_setg(errp, "Protocol error: NBD_REPLY_TYPE_NONE chunk without"
+                             "NBD_REPLY_FLAG_DONE flag set");
+            return -EINVAL;
+        }
+        return 0;
+    }
+
+    if (chunk->type == NBD_REPLY_TYPE_OFFSET_DATA) {
+        if (!qiov) {
+            error_setg(errp, "Unexpected NBD_REPLY_TYPE_OFFSET_DATA chunk");
+            return -EINVAL;
+        }
+
+        return nbd_co_receive_offset_data_payload(s, qiov, errp);
+    }
+
+    if (nbd_reply_type_is_error(chunk->type)) {
+        payload = &local_payload;
+    }
+
+    ret = nbd_co_receive_structured_payload(s, payload, errp);
+    if (ret < 0) {
+        return ret;
+    }
+
+    if (nbd_reply_type_is_error(chunk->type)) {
+        ret = nbd_parse_error_payload(chunk, local_payload, request_ret, errp);
+        qemu_vfree(local_payload);
+        return ret;
+    }
+
+    return 0;
+}
+
+/* nbd_co_receive_one_chunk
+ * Read reply, wake up read_reply_co and set s->quit if needed.
+ * Return value is a fatal error code or normal nbd reply error code
+ *
+ * The pointer stored in @payload requires qemu_vfree() to free it.
+ */
+static coroutine_fn int nbd_co_receive_one_chunk(
+        NBDClientSession *s, uint64_t handle, bool only_structured,
+        QEMUIOVector *qiov, NBDReply *reply, void **payload, Error **errp)
+{
+    int request_ret;
+    int ret = nbd_co_do_receive_one_chunk(s, handle, only_structured,
+                                          &request_ret, qiov, payload, errp);
+
+    if (ret < 0) {
+        s->quit = true;
+    } else {
+        /* For assert at loop start in nbd_read_reply_entry */
+        if (reply) {
+            *reply = s->reply;
+        }
+        s->reply.handle = 0;
+        ret = request_ret;
+    }
 
-    /* Kick the read_reply_co to get the next reply.  */
     if (s->read_reply_co) {
         aio_co_wake(s->read_reply_co);
     }
 
+    return ret;
+}
+
+typedef struct NBDReplyChunkIter {
+    int ret;
+    Error *err;
+    bool done, only_structured;
+} NBDReplyChunkIter;
+
+static void nbd_iter_error(NBDReplyChunkIter *iter, bool fatal,
+                           int ret, Error **local_err)
+{
+    assert(ret < 0);
+
+    if (fatal || iter->ret == 0) {
+        if (iter->ret != 0) {
+            error_free(iter->err);
+            iter->err = NULL;
+        }
+        iter->ret = ret;
+        error_propagate(&iter->err, *local_err);
+    } else {
+        error_free(*local_err);
+    }
+
+    *local_err = NULL;
+}
+
+/* NBD_FOREACH_REPLY_CHUNK
+ * The pointer stored in @payload requires qemu_vfree() to free it.
+ */
+#define NBD_FOREACH_REPLY_CHUNK(s, iter, handle, structured, \
+                                qiov, reply, payload) \
+    for (iter = (NBDReplyChunkIter) { .only_structured = structured }; \
+         nbd_reply_chunk_iter_receive(s, &iter, handle, qiov, reply, payload);)
+
+/* nbd_reply_chunk_iter_receive
+ * The pointer stored in @payload requires qemu_vfree() to free it.
+ */
+static bool nbd_reply_chunk_iter_receive(NBDClientSession *s,
+                                         NBDReplyChunkIter *iter,
+                                         uint64_t handle,
+                                         QEMUIOVector *qiov, NBDReply *reply,
+                                         void **payload)
+{
+    int ret;
+    NBDReply local_reply;
+    NBDStructuredReplyChunk *chunk;
+    Error *local_err = NULL;
+    if (s->quit) {
+        error_setg(&local_err, "Connection closed");
+        nbd_iter_error(iter, true, -EIO, &local_err);
+        goto break_loop;
+    }
+
+    if (iter->done) {
+        /* Previous iteration was last. */
+        goto break_loop;
+    }
+
+    if (reply == NULL) {
+        reply = &local_reply;
+    }
+
+    ret = nbd_co_receive_one_chunk(s, handle, iter->only_structured,
+                                   qiov, reply, payload, &local_err);
+    if (ret < 0) {
+        /* If it is a fatal error s->quit is set by nbd_co_receive_one_chunk */
+        nbd_iter_error(iter, s->quit, ret, &local_err);
+    }
+
+    /* Do not execute the body of NBD_FOREACH_REPLY_CHUNK for simple reply. */
+    if (nbd_reply_is_simple(&s->reply) || s->quit) {
+        goto break_loop;
+    }
+
+    chunk = &reply->structured;
+    iter->only_structured = true;
+
+    if (chunk->type == NBD_REPLY_TYPE_NONE) {
+        /* NBD_REPLY_FLAG_DONE is already checked in nbd_co_receive_one_chunk */
+        assert(chunk->flags & NBD_REPLY_FLAG_DONE);
+        goto break_loop;
+    }
+
+    if (chunk->flags & NBD_REPLY_FLAG_DONE) {
+        /* This iteration is last. */
+        iter->done = true;
+    }
+
+    /* Execute the loop body */
+    return true;
+
+break_loop:
+    s->requests[HANDLE_TO_INDEX(s, handle)].coroutine = NULL;
+
     qemu_co_mutex_lock(&s->send_mutex);
     s->in_flight--;
     qemu_co_queue_next(&s->free_sema);
     qemu_co_mutex_unlock(&s->send_mutex);
 
-    return ret;
+    return false;
 }
 
-static int nbd_co_request(BlockDriverState *bs,
-                          NBDRequest *request,
-                          QEMUIOVector *qiov)
+static int nbd_co_receive_return_code(NBDClientSession *s, uint64_t handle,
+                                      Error **errp)
+{
+    NBDReplyChunkIter iter;
+
+    NBD_FOREACH_REPLY_CHUNK(s, iter, handle, false, NULL, NULL, NULL) {
+        /* nbd_reply_chunk_iter_receive does all the work */
+        ;
+    }
+
+    error_propagate(errp, iter.err);
+    return iter.ret;
+}
+
+static int nbd_co_receive_cmdread_reply(NBDClientSession *s, uint64_t handle,
+                                        QEMUIOVector *qiov, Error **errp)
+{
+    NBDReplyChunkIter iter;
+    NBDReply reply;
+    void *payload = NULL;
+    Error *local_err = NULL;
+
+    NBD_FOREACH_REPLY_CHUNK(s, iter, handle, s->info.structured_reply,
+                            qiov, &reply, &payload)
+    {
+        int ret;
+        NBDStructuredReplyChunk *chunk = &reply.structured;
+
+        assert(nbd_reply_is_structured(&reply));
+
+        switch (chunk->type) {
+        case NBD_REPLY_TYPE_OFFSET_DATA:
+            /* special cased in nbd_co_receive_one_chunk, data is already
+             * in qiov */
+            break;
+        case NBD_REPLY_TYPE_OFFSET_HOLE:
+            ret = nbd_parse_offset_hole_payload(&reply.structured, payload,
+                                                qiov, &local_err);
+            if (ret < 0) {
+                s->quit = true;
+                nbd_iter_error(&iter, true, ret, &local_err);
+            }
+            break;
+        default:
+            if (!nbd_reply_type_is_error(chunk->type)) {
+                /* not allowed reply type */
+                s->quit = true;
+                error_setg(&local_err,
+                           "Unexpected reply type: %d (%s) for CMD_READ",
+                           chunk->type, nbd_reply_type_lookup(chunk->type));
+                nbd_iter_error(&iter, true, -EINVAL, &local_err);
+            }
+        }
+
+        qemu_vfree(payload);
+        payload = NULL;
+    }
+
+    error_propagate(errp, iter.err);
+    return iter.ret;
+}
+
+static int nbd_co_request(BlockDriverState *bs, NBDRequest *request,
+                          QEMUIOVector *write_qiov)
 {
-    NBDClientSession *client = nbd_get_client_session(bs);
     int ret;
+    Error *local_err = NULL;
+    NBDClientSession *client = nbd_get_client_session(bs);
 
-    if (qiov) {
-        assert(request->type == NBD_CMD_WRITE || request->type == NBD_CMD_READ);
-        assert(request->len == iov_size(qiov->iov, qiov->niov));
+    assert(request->type != NBD_CMD_READ);
+    if (write_qiov) {
+        assert(request->type == NBD_CMD_WRITE);
+        assert(request->len == iov_size(write_qiov->iov, write_qiov->niov));
     } else {
-        assert(request->type != NBD_CMD_WRITE && request->type != NBD_CMD_READ);
+        assert(request->type != NBD_CMD_WRITE);
     }
-    ret = nbd_co_send_request(bs, request,
-                              request->type == NBD_CMD_WRITE ? qiov : NULL);
+    ret = nbd_co_send_request(bs, request, write_qiov);
     if (ret < 0) {
         return ret;
     }
 
-    return nbd_co_receive_reply(client, request->handle,
-                                request->type == NBD_CMD_READ ? qiov : NULL);
+    ret = nbd_co_receive_return_code(client, request->handle, &local_err);
+    if (ret < 0) {
+        error_report_err(local_err);
+    }
+    return ret;
 }
 
 int nbd_client_co_preadv(BlockDriverState *bs, uint64_t offset,
                          uint64_t bytes, QEMUIOVector *qiov, int flags)
 {
+    int ret;
+    Error *local_err = NULL;
+    NBDClientSession *client = nbd_get_client_session(bs);
     NBDRequest request = {
         .type = NBD_CMD_READ,
         .from = offset,
@@ -259,7 +670,16 @@ int nbd_client_co_preadv(BlockDriverState *bs, uint64_t offset,
     assert(bytes <= NBD_MAX_BUFFER_SIZE);
     assert(!flags);
 
-    return nbd_co_request(bs, &request, qiov);
+    ret = nbd_co_send_request(bs, &request, NULL);
+    if (ret < 0) {
+        return ret;
+    }
+
+    ret = nbd_co_receive_cmdread_reply(client, request.handle, qiov, &local_err);
+    if (ret < 0) {
+        error_report_err(local_err);
+    }
+    return ret;
 }
 
 int nbd_client_co_pwritev(BlockDriverState *bs, uint64_t offset,
@@ -381,6 +801,7 @@ int nbd_client_init(BlockDriverState *bs,
     qio_channel_set_blocking(QIO_CHANNEL(sioc), true, NULL);
 
     client->info.request_sizes = true;
+    client->info.structured_reply = true;
     ret = nbd_receive_negotiate(QIO_CHANNEL(sioc), export,
                                 tlscreds, hostname,
                                 &client->ioc, &client->info, errp);
diff --git a/nbd/client.c b/nbd/client.c
index eeb23ae934..0297484264 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -684,6 +684,16 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name,
         if (fixedNewStyle) {
             int result;
 
+            if (info->structured_reply) {
+                result = nbd_request_simple_option(ioc,
+                                                   NBD_OPT_STRUCTURED_REPLY,
+                                                   errp);
+                if (result < 0) {
+                    goto fail;
+                }
+                info->structured_reply = result == 1;
+            }
+
             /* Try NBD_OPT_GO first - if it works, we are done (it
              * also gives us a good message if the server requires
              * TLS).  If it is not available, fall back to
-- 
2.11.1

Re: [Qemu-devel] [PATCH v4 RFC 9/8] nbd: Minimal structured read for client

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 19643 bytes --]

On 10/17/2017 07:57 AM, Vladimir Sementsov-Ogievskiy wrote:
> Minimal implementation: for structured error only error_report error
> message.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> 

> 
> RFC: I don't like resulting error handling: it is rather confusing,
>     requires a lot of definitions of "local_err" which looks bad. Propagating,
>     setting error and then drop it for errp=NULL case - all it looks not
>     very effective. Any ideas?

As long as the function you call has a sane return value that can easily
be used to tell if you had an error or not, you can often use that to
pass errp through the call, rather than needing a local_err and
error_propagate().  I'll see if I can spot such places during my review.

>  include/block/nbd.h |  12 ++
>  nbd/nbd-internal.h  |   1 -
>  block/nbd-client.c  | 483 ++++++++++++++++++++++++++++++++++++++++++++++++----
>  nbd/client.c        |  10 ++
>  4 files changed, 474 insertions(+), 32 deletions(-)

Feels like a big patch.  Of course, we can't enable the client
requesting structured replies until all the pieces are in place
(otherwise, it becomes difficult to bisect a broken client against a
working server); but it may be possible to split this patch into smaller
pieces if it would aid review.  That said, I'm fine keeping it as one
big patch, if I don't see too much that needs tweaking.

First, before I review anything, a quick check, with this patch applied:

Server:
$ ./qemu-nbd -x foo -f qcow2 --trace='nbd_*' file -r

Client:
$ ./qemu-io -f raw nbd://localhost:10809/foo --trace='nbd_*'

Looking at the trace, the negotiation succeeds (yay!).

But in the client, I then perform 'w 0 0' (a zero-byte write, which
should fail because the server is read-only).  I see:

C: 19481@1508268433.381446:nbd_send_request Sending request to server: {
.from = 0, .len = 0, .handle = 93997172956880, .flags = 0x1, .type = 1
(write) }
S: 19479@1508268433.381516:nbd_receive_request Got request: { magic =
0x25609513, .flags = 0x1, .type = 0x1, from = 0, len = 0 }
S: 19479@1508268433.381527:nbd_co_receive_request_decode_type Decoding
type: handle = 93997172956880, type = 1 (write)
S: 19479@1508268433.381540:nbd_co_receive_request_payload_received
Payload received: handle = 93997172956880, len = 0
S: 19479@1508268433.381564:nbd_co_send_structured_error Send structured
error reply: handle = 93997172956880, error = 1 (EPERM), msg = ''
C: 19481@1508268433.381622:nbd_receive_structured_reply_chunk Got
structured reply chunk: { flags = 0x1, type = 32769, handle =
93997172956880, length = 6 }
C: wrote 0/0 bytes at offset 0
C: 0 bytes, 1 ops; 0.0002 sec (0 bytes/sec and 4291.8455 ops/sec)

Oops - the client claimed success, even though the server replied with
EPERM.  And the server didn't do a good job of including details on the
error message.  So there's still some tweaks needed.



> @@ -181,75 +181,486 @@ err:
>      return rc;
>  }
>  
> -static int nbd_co_receive_reply(NBDClientSession *s,
> -                                uint64_t handle,
> -                                QEMUIOVector *qiov)
> +static inline uint16_t payload_advance16(uint8_t **payload)
> +{
> +    *payload += 2;
> +    return lduw_be_p(*payload - 2);

Correct - loads a 16-bit unsigned quantity.

> +}
> +
> +static inline uint32_t payload_advance32(uint8_t **payload)
> +{
> +    *payload += 4;
> +    return lduw_be_p(*payload - 4);

Oops, you want ldl_be_p(), to load a 32-bit quantity (sign not relevant).

> +}
> +
> +static inline uint64_t payload_advance64(uint8_t **payload)
> +{
> +    *payload += 8;
> +    return lduw_be_p(*payload - 8);

Oops, you want ldq_be_p(), to load a 64-bit quantity.  Peter's pending
patch "[PATCH v4] docs/devel/loads-stores.rst: Document our various load
and store APIs" goes into more details on which load/store routines to
use when.

> +
> +static int nbd_co_receive_offset_data_payload(NBDClientSession *s,
> +                                              QEMUIOVector *qiov, Error **errp)
> +{
> +    QEMUIOVector sub_qiov;
> +    uint64_t offset;
> +    size_t data_size;
> +    int ret;
> +    NBDStructuredReplyChunk *chunk = &s->reply.structured;
> +
> +    assert(nbd_reply_is_structured(&s->reply));
> +
> +    if (chunk->length < sizeof(offset)) {
> +        error_setg(errp, "Protocol error: invalid payload for "
> +                         "NBD_REPLY_TYPE_OFFSET_DATA");
> +        return -EINVAL;
> +    }
> +
> +    if (nbd_read(s->ioc, &offset, sizeof(offset), errp) < 0) {
> +        return -EIO;
> +    }
> +    be64_to_cpus(&offset);
> +
> +    data_size = chunk->length - sizeof(offset);
> +    if (offset + data_size > qiov->size) {

Arithmetic overflow is possible here; it's safer to write:

if (offset > qiov->size - data_size)

Similar for hole_size earlier.

> +
> +#define NBD_MAX_MALLOC_PAYLOAD 1000
> +/* nbd_co_receive_structured_payload
> + * The resulting pointer stored in @payload requires qemu_vfree() to free it.
> + */
> +static coroutine_fn int nbd_co_receive_structured_payload(
> +        NBDClientSession *s, void **payload, Error **errp)
> +{
> +    int ret;
> +    uint32_t len;
> +
> +    assert(nbd_reply_is_structured(&s->reply));
> +
> +    len = s->reply.structured.length;
> +
> +    if (len == 0) {
> +        return 0;
> +    }

Should we assign *payload to NULL even on this short-circuit (if payload
itself is non-NULL)?  Oh, you do this in the caller [1]

> +
> +    if (payload == NULL) {
> +        error_setg(errp, "Unexpected structured payload");
> +        return -EINVAL;
> +    }
> +
> +    if (len > NBD_MAX_MALLOC_PAYLOAD) {
> +        error_setg(errp, "Too large payload");

Grammar: Payload too large

> +        return -EINVAL;
> +    }
> +
> +    *payload = qemu_memalign(8, len);

Why are we bothering to read this into memalign'ed memory, seeing as how
it is packed data, and more likely than not, we will NOT be benefitting
from the alignment?  Can we get by with simpler g_new()/g_free()?

> +    ret = nbd_read(s->ioc, *payload, len, errp);
> +    if (ret < 0) {
> +        qemu_vfree(*payload);
> +        *payload = NULL;
> +        return ret;
> +    }
> +
> +    return 0;
> +}
> +
> +/* nbd_co_do_receive_one_chunk
> + * for simple reply:
> + *   set request_ret to received reply error
> + *   if qiov is not NULL: read payload to @qiov
> + * for structured reply chunk:
> + *   if error chunk: read payload, set @request_ret, do not set @payload
> + *   else if offset_data chunk: read payload data to @qiov, do not set @payload
> + *   else: read payload to @payload
> + *
> + * The pointer stored in @payload requires qemu_vfree() to free it.
> + * If function fails @errp containts corresponding error message. If function

s/containts/contains/

> + * doesn't fail but reply contains error @request_ret is set to corresponding
> + * return code and errp is set to corresponding error message.

Hmm, that feels awkward - if errp can be set even when the function does
not return a negative value, then it's tougher to tell failures apart.

> + */
> +static coroutine_fn int nbd_co_do_receive_one_chunk(
> +        NBDClientSession *s, uint64_t handle, bool only_structured,
> +        int *request_ret, QEMUIOVector *qiov, void **payload, Error **errp)
>  {
>      int ret;
>      int i = HANDLE_TO_INDEX(s, handle);
> +    void *local_payload = NULL;
> +    NBDStructuredReplyChunk *chunk;
> +
> +    if (payload) {
> +        *payload = NULL;
> +    }

[1] answer to my question above.

> +
> +/* nbd_co_receive_one_chunk
> + * Read reply, wake up read_reply_co and set s->quit if needed.
> + * Return value is a fatal error code or normal nbd reply error code
> + *
> + * The pointer stored in @payload requires qemu_vfree() to free it.
> + */
> +static coroutine_fn int nbd_co_receive_one_chunk(
> +        NBDClientSession *s, uint64_t handle, bool only_structured,
> +        QEMUIOVector *qiov, NBDReply *reply, void **payload, Error **errp)
> +{
> +    int request_ret;
> +    int ret = nbd_co_do_receive_one_chunk(s, handle, only_structured,
> +                                          &request_ret, qiov, payload, errp);
> +
> +    if (ret < 0) {
> +        s->quit = true;
> +    } else {
> +        /* For assert at loop start in nbd_read_reply_entry */
> +        if (reply) {
> +            *reply = s->reply;
> +        }
> +        s->reply.handle = 0;
> +        ret = request_ret;
> +    }
>  
> -    /* Kick the read_reply_co to get the next reply.  */
>      if (s->read_reply_co) {
>          aio_co_wake(s->read_reply_co);
>      }

Some things are a bit harder to test as long as our only working server
is currently hardcoded to send exactly one chunk per message; but that's
part of the testing I plan to do in the next couple of days (even if I
end up using gdb to fake packet sequences rather than actually coding a
more-complete server).

>  
> +    return ret;
> +}
> +
> +typedef struct NBDReplyChunkIter {
> +    int ret;
> +    Error *err;
> +    bool done, only_structured;
> +} NBDReplyChunkIter;
> +
> +static void nbd_iter_error(NBDReplyChunkIter *iter, bool fatal,
> +                           int ret, Error **local_err)
> +{
> +    assert(ret < 0);
> +
> +    if (fatal || iter->ret == 0) {
> +        if (iter->ret != 0) {
> +            error_free(iter->err);
> +            iter->err = NULL;
> +        }
> +        iter->ret = ret;
> +        error_propagate(&iter->err, *local_err);

error_propagate() can be safely called more than once (only the first
call sets an error).  I guess what you're trying to do here is that if
the server sends more than one error chunk, you favor that message; but
if we fail to communicate with the server, that takes even higher priority.

> +    } else {
> +        error_free(*local_err);
> +    }
> +
> +    *local_err = NULL;
> +}
> +
> +/* NBD_FOREACH_REPLY_CHUNK
> + * The pointer stored in @payload requires qemu_vfree() to free it.
> + */
> +#define NBD_FOREACH_REPLY_CHUNK(s, iter, handle, structured, \
> +                                qiov, reply, payload) \
> +    for (iter = (NBDReplyChunkIter) { .only_structured = structured }; \
> +         nbd_reply_chunk_iter_receive(s, &iter, handle, qiov, reply, payload);)
> +
> +/* nbd_reply_chunk_iter_receive
> + * The pointer stored in @payload requires qemu_vfree() to free it.
> + */

Maybe worth a comment documenting the return value?  Although it is
somewhat mentioned below [2]

> +static bool nbd_reply_chunk_iter_receive(NBDClientSession *s,
> +                                         NBDReplyChunkIter *iter,
> +                                         uint64_t handle,
> +                                         QEMUIOVector *qiov, NBDReply *reply,
> +                                         void **payload)
> +{
> +    int ret;
> +    NBDReply local_reply;
> +    NBDStructuredReplyChunk *chunk;
> +    Error *local_err = NULL;
> +    if (s->quit) {
> +        error_setg(&local_err, "Connection closed");
> +        nbd_iter_error(iter, true, -EIO, &local_err);
> +        goto break_loop;
> +    }
> +
> +    if (iter->done) {
> +        /* Previous iteration was last. */
> +        goto break_loop;
> +    }
> +
> +    if (reply == NULL) {
> +        reply = &local_reply;
> +    }
> +
> +    ret = nbd_co_receive_one_chunk(s, handle, iter->only_structured,
> +                                   qiov, reply, payload, &local_err);
> +    if (ret < 0) {
> +        /* If it is a fatal error s->quit is set by nbd_co_receive_one_chunk */
> +        nbd_iter_error(iter, s->quit, ret, &local_err);
> +    }
> +
> +    /* Do not execute the body of NBD_FOREACH_REPLY_CHUNK for simple reply. */
> +    if (nbd_reply_is_simple(&s->reply) || s->quit) {
> +        goto break_loop;
> +    }
> +
> +    chunk = &reply->structured;
> +    iter->only_structured = true;
> +
> +    if (chunk->type == NBD_REPLY_TYPE_NONE) {
> +        /* NBD_REPLY_FLAG_DONE is already checked in nbd_co_receive_one_chunk */
> +        assert(chunk->flags & NBD_REPLY_FLAG_DONE);
> +        goto break_loop;
> +    }
> +
> +    if (chunk->flags & NBD_REPLY_FLAG_DONE) {
> +        /* This iteration is last. */
> +        iter->done = true;
> +    }
> +
> +    /* Execute the loop body */
> +    return true;
> +

[2]

> +break_loop:
> +    s->requests[HANDLE_TO_INDEX(s, handle)].coroutine = NULL;
> +
>      qemu_co_mutex_lock(&s->send_mutex);
>      s->in_flight--;
>      qemu_co_queue_next(&s->free_sema);
>      qemu_co_mutex_unlock(&s->send_mutex);
>  
> -    return ret;
> +    return false;
>  }
>  
> -static int nbd_co_request(BlockDriverState *bs,
> -                          NBDRequest *request,
> -                          QEMUIOVector *qiov)
> +static int nbd_co_receive_return_code(NBDClientSession *s, uint64_t handle,
> +                                      Error **errp)
> +{
> +    NBDReplyChunkIter iter;
> +
> +    NBD_FOREACH_REPLY_CHUNK(s, iter, handle, false, NULL, NULL, NULL) {
> +        /* nbd_reply_chunk_iter_receive does all the work */
> +        ;

Do we need the empty ';'?  {} is a valid loop body.

> +    }
> +
> +    error_propagate(errp, iter.err);
> +    return iter.ret;
> +}
> +
> +static int nbd_co_receive_cmdread_reply(NBDClientSession *s, uint64_t handle,
> +                                        QEMUIOVector *qiov, Error **errp)
> +{
> +    NBDReplyChunkIter iter;
> +    NBDReply reply;
> +    void *payload = NULL;
> +    Error *local_err = NULL;
> +
> +    NBD_FOREACH_REPLY_CHUNK(s, iter, handle, s->info.structured_reply,
> +                            qiov, &reply, &payload)
> +    {
> +        int ret;
> +        NBDStructuredReplyChunk *chunk = &reply.structured;
> +
> +        assert(nbd_reply_is_structured(&reply));
> +
> +        switch (chunk->type) {
> +        case NBD_REPLY_TYPE_OFFSET_DATA:
> +            /* special cased in nbd_co_receive_one_chunk, data is already
> +             * in qiov */
> +            break;
> +        case NBD_REPLY_TYPE_OFFSET_HOLE:
> +            ret = nbd_parse_offset_hole_payload(&reply.structured, payload,
> +                                                qiov, &local_err);
> +            if (ret < 0) {
> +                s->quit = true;
> +                nbd_iter_error(&iter, true, ret, &local_err);
> +            }
> +            break;
> +        default:
> +            if (!nbd_reply_type_is_error(chunk->type)) {
> +                /* not allowed reply type */
> +                s->quit = true;
> +                error_setg(&local_err,
> +                           "Unexpected reply type: %d (%s) for CMD_READ",
> +                           chunk->type, nbd_reply_type_lookup(chunk->type));
> +                nbd_iter_error(&iter, true, -EINVAL, &local_err);

This feels a bit like action at a difference - breaking out of the loop
but making sure the loop still tracks the right error.  But I'm not
immediately seeing if it is safe to directly use 'return' here instead.

> +            }
> +        }
> +
> +        qemu_vfree(payload);
> +        payload = NULL;
> +    }
> +
> +    error_propagate(errp, iter.err);
> +    return iter.ret;
> +}
> +
> +static int nbd_co_request(BlockDriverState *bs, NBDRequest *request,
> +                          QEMUIOVector *write_qiov)
>  {
> -    NBDClientSession *client = nbd_get_client_session(bs);
>      int ret;
> +    Error *local_err = NULL;
> +    NBDClientSession *client = nbd_get_client_session(bs);
>  
> -    if (qiov) {
> -        assert(request->type == NBD_CMD_WRITE || request->type == NBD_CMD_READ);
> -        assert(request->len == iov_size(qiov->iov, qiov->niov));
> +    assert(request->type != NBD_CMD_READ);
> +    if (write_qiov) {
> +        assert(request->type == NBD_CMD_WRITE);
> +        assert(request->len == iov_size(write_qiov->iov, write_qiov->niov));
>      } else {
> -        assert(request->type != NBD_CMD_WRITE && request->type != NBD_CMD_READ);
> +        assert(request->type != NBD_CMD_WRITE);
>      }
> -    ret = nbd_co_send_request(bs, request,
> -                              request->type == NBD_CMD_WRITE ? qiov : NULL);
> +    ret = nbd_co_send_request(bs, request, write_qiov);

If we have to respin things, wiring up errp handling to existing calls
prior to adding structured reply handling would probably be worth
splitting the patch.

>      if (ret < 0) {
>          return ret;
>      }
>  
> -    return nbd_co_receive_reply(client, request->handle,
> -                                request->type == NBD_CMD_READ ? qiov : NULL);
> +    ret = nbd_co_receive_return_code(client, request->handle, &local_err);
> +    if (ret < 0) {
> +        error_report_err(local_err);
> +    }
> +    return ret;
>  }
>  
>  int nbd_client_co_preadv(BlockDriverState *bs, uint64_t offset,
>                           uint64_t bytes, QEMUIOVector *qiov, int flags)
>  {
> +    int ret;
> +    Error *local_err = NULL;
> +    NBDClientSession *client = nbd_get_client_session(bs);
>      NBDRequest request = {
>          .type = NBD_CMD_READ,
>          .from = offset,
> @@ -259,7 +670,16 @@ int nbd_client_co_preadv(BlockDriverState *bs, uint64_t offset,
>      assert(bytes <= NBD_MAX_BUFFER_SIZE);
>      assert(!flags);
>  
> -    return nbd_co_request(bs, &request, qiov);
> +    ret = nbd_co_send_request(bs, &request, NULL);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    ret = nbd_co_receive_cmdread_reply(client, request.handle, qiov, &local_err);
> +    if (ret < 0) {
> +        error_report_err(local_err);
> +    }
> +    return ret;
>  }
>  
>  int nbd_client_co_pwritev(BlockDriverState *bs, uint64_t offset,
> @@ -381,6 +801,7 @@ int nbd_client_init(BlockDriverState *bs,
>      qio_channel_set_blocking(QIO_CHANNEL(sioc), true, NULL);
>  
>      client->info.request_sizes = true;
> +    client->info.structured_reply = true;
>      ret = nbd_receive_negotiate(QIO_CHANNEL(sioc), export,
>                                  tlscreds, hostname,
>                                  &client->ioc, &client->info, errp);
> diff --git a/nbd/client.c b/nbd/client.c
> index eeb23ae934..0297484264 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -684,6 +684,16 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name,
>          if (fixedNewStyle) {
>              int result;
>  
> +            if (info->structured_reply) {
> +                result = nbd_request_simple_option(ioc,
> +                                                   NBD_OPT_STRUCTURED_REPLY,
> +                                                   errp);
> +                if (result < 0) {
> +                    goto fail;
> +                }
> +                info->structured_reply = result == 1;
> +            }

Looks like the handshake is handled correctly.

> +
>              /* Try NBD_OPT_GO first - if it works, we are done (it
>               * also gives us a good message if the server requires
>               * TLS).  If it is not available, fall back to
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v4 RFC 9/8] nbd: Minimal structured read for client

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1937 bytes --]

On 10/17/2017 04:17 PM, Eric Blake wrote:
> On 10/17/2017 07:57 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Minimal implementation: for structured error only error_report error
>> message.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---

> But in the client, I then perform 'w 0 0' (a zero-byte write, which
> should fail because the server is read-only).  I see:
> 
> C: 19481@1508268433.381446:nbd_send_request Sending request to server: {
> .from = 0, .len = 0, .handle = 93997172956880, .flags = 0x1, .type = 1
> (write) }
> S: 19479@1508268433.381516:nbd_receive_request Got request: { magic =
> 0x25609513, .flags = 0x1, .type = 0x1, from = 0, len = 0 }
> S: 19479@1508268433.381527:nbd_co_receive_request_decode_type Decoding
> type: handle = 93997172956880, type = 1 (write)
> S: 19479@1508268433.381540:nbd_co_receive_request_payload_received
> Payload received: handle = 93997172956880, len = 0
> S: 19479@1508268433.381564:nbd_co_send_structured_error Send structured
> error reply: handle = 93997172956880, error = 1 (EPERM), msg = ''
> C: 19481@1508268433.381622:nbd_receive_structured_reply_chunk Got
> structured reply chunk: { flags = 0x1, type = 32769, handle =
> 93997172956880, length = 6 }
> C: wrote 0/0 bytes at offset 0
> C: 0 bytes, 1 ops; 0.0002 sec (0 bytes/sec and 4291.8455 ops/sec)
> 
> Oops - the client claimed success, even though the server replied with
> EPERM.  And the server didn't do a good job of including details on the
> error message.  So there's still some tweaks needed.

The server not sending details is a separate pre-existing issue; in
server.c, we set errp for NBD_CMD_WRITE_ZEROES but not for
NBD_CMD_WRITE.  You can get the server to cough up a message by using 'w
-z 0 1' instead.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v4 RFC 9/8] nbd: Minimal structured read for client

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 5643 bytes --]

On 10/17/2017 07:57 AM, Vladimir Sementsov-Ogievskiy wrote:
> Minimal implementation: for structured error only error_report error
> message.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---

and I replied:
> 
> But in the client, I then perform 'w 0 0' (a zero-byte write, which
> should fail because the server is read-only).  I see:
> 
> C: 19481@1508268433.381446:nbd_send_request Sending request to server: {
> .from = 0, .len = 0, .handle = 93997172956880, .flags = 0x1, .type = 1
> (write) }
> S: 19479@1508268433.381516:nbd_receive_request Got request: { magic =
> 0x25609513, .flags = 0x1, .type = 0x1, from = 0, len = 0 }
> S: 19479@1508268433.381527:nbd_co_receive_request_decode_type Decoding
> type: handle = 93997172956880, type = 1 (write)
> S: 19479@1508268433.381540:nbd_co_receive_request_payload_received
> Payload received: handle = 93997172956880, len = 0
> S: 19479@1508268433.381564:nbd_co_send_structured_error Send structured
> error reply: handle = 93997172956880, error = 1 (EPERM), msg = ''
> C: 19481@1508268433.381622:nbd_receive_structured_reply_chunk Got
> structured reply chunk: { flags = 0x1, type = 32769, handle =
> 93997172956880, length = 6 }
> C: wrote 0/0 bytes at offset 0
> C: 0 bytes, 1 ops; 0.0002 sec (0 bytes/sec and 4291.8455 ops/sec)
> 
> Oops - the client claimed success, even though the server replied with
> EPERM.  And the server didn't do a good job of including details on the
> error message.  So there's still some tweaks needed.

Okay, I found that issue:

> +static int nbd_parse_error_payload(NBDStructuredReplyChunk *chunk,
> +                                   uint8_t *payload, int *request_ret,
> +                                   Error **errp)
> +{
> +    uint32_t error;
> +    uint16_t message_size;
> +
> +    assert(chunk->type & (1 << 15));
> +
> +    if (chunk->length < sizeof(error) + sizeof(message_size)) {
> +        error_setg(errp,
> +                   "Protocol error: invalid payload for structured error");
> +        return -EINVAL;
> +    }
> +
> +    error = nbd_errno_to_system_errno(payload_advance32(&payload));
> +    if (error == 0) {
> +        error_setg(errp, "Protocol error: server sent structured error chunk"
> +                         "with error = 0");
> +        return -EINVAL;
> +    }
> +
> +    *request_ret = error;

Here, you set *request_ret to a positive value when the server gives an
error,

> +static coroutine_fn int nbd_co_do_receive_one_chunk(
> +        NBDClientSession *s, uint64_t handle, bool only_structured,
> +        int *request_ret, QEMUIOVector *qiov, void **payload, Error **errp)
>  {

> -    } else {
> -        assert(s->reply.handle == handle);
> -        ret = -nbd_errno_to_system_errno(s->reply.simple.error);
> -        if (qiov && ret == 0) {
> -            if (qio_channel_readv_all(s->ioc, qiov->iov, qiov->niov,
> -                                      NULL) < 0) {
> -                ret = -EIO;
> -                s->quit = true;
> +        error_setg(errp, "Connection closed");
> +        return -EIO;
> +    }
> +
> +    assert(s->reply.handle == handle);
> +
> +    if (nbd_reply_is_simple(&s->reply)) {
> +        if (only_structured) {
> +            error_setg(errp, "Protocol error: simple reply when structured"
> +                             "reply chunk was expected");
> +            return -EINVAL;
> +        }
> +
> +        *request_ret = -nbd_errno_to_system_errno(s->reply.simple.error);

But here, you set it to a negative value,

> +/* nbd_reply_chunk_iter_receive
> + * The pointer stored in @payload requires qemu_vfree() to free it.
> + */
> +static bool nbd_reply_chunk_iter_receive(NBDClientSession *s,
> +                                         NBDReplyChunkIter *iter,
> +                                         uint64_t handle,
> +                                         QEMUIOVector *qiov, NBDReply *reply,
> +                                         void **payload)
> +{
> +    int ret;
> +    NBDReply local_reply;
> +    NBDStructuredReplyChunk *chunk;
> +    Error *local_err = NULL;
> +    if (s->quit) {
> +        error_setg(&local_err, "Connection closed");
> +        nbd_iter_error(iter, true, -EIO, &local_err);
> +        goto break_loop;
> +    }
> +
> +    if (iter->done) {
> +        /* Previous iteration was last. */
> +        goto break_loop;
> +    }
> +
> +    if (reply == NULL) {
> +        reply = &local_reply;
> +    }
> +
> +    ret = nbd_co_receive_one_chunk(s, handle, iter->only_structured,
> +                                   qiov, reply, payload, &local_err);
> +    if (ret < 0) {
> +        /* If it is a fatal error s->quit is set by nbd_co_receive_one_chunk */
> +        nbd_iter_error(iter, s->quit, ret, &local_err);
> +    }

and you only ever set iter.ret to non-zero if the value was negative (so
you were missing all errors sent through a structured reply).

There was a lot of back-and-forth hunting through the code to see where
errors flow.  I wonder if our intermediate processing should try harder
to distinguish between NBD errors sent by the server (but the connection
is still good - aka not a fatal error for the receive loop but does
affect the end client) vs. those detected locally (server sent garbage
or our connection failed, either way, the error is fatal and we won't
ever talk to the server again).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v4 RFC 9/8] nbd: Minimal structured read for client

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2322 bytes --]

On 10/17/2017 07:57 AM, Vladimir Sementsov-Ogievskiy wrote:
> Minimal implementation: for structured error only error_report error
> message.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> 

> +static int nbd_co_request(BlockDriverState *bs, NBDRequest *request,
> +                          QEMUIOVector *write_qiov)
>  {

> -    return nbd_co_receive_reply(client, request->handle,
> -                                request->type == NBD_CMD_READ ? qiov : NULL);
> +    ret = nbd_co_receive_return_code(client, request->handle, &local_err);
> +    if (ret < 0) {
> +        error_report_err(local_err);
> +    }

I think this new error_report_err() is a regression in behavior.
Running the old server:

$ qemu-nbd -x foo -f qcow2 --trace='nbd_*' file -r

and an old client:

$ qemu-io -f raw nbd://localhost:10809/foo
qemu-io> w 0 0
write failed: Operation not permitted
qemu-io> q

but with the new client (once I fix the bug about being able to ignore
the NBD_REP_ERR_UNSUP with non-zero length in the earlier patch):

$ ./qemu-io -f raw nbd://localhost:10809/foo
qemu-io> w 0 0
Request failed: Operation not permitted
write failed: Operation not permitted
qemu-io>

and worse, new server with new client:

$ ./qemu-io -f raw nbd://localhost:10809/foo
qemu-io> w 0 0
: Operation not permitted
write failed: Operation not permitted
qemu-io>

we don't even manage to post a sane message.

Reporting fatal errors where we lose connection with the server (or
forcefully give up on the server because it violated protocol) may be
okay, but reporting common errors where the server reported a problem
but we are still connected is too verbose.

I know I asked about errp plumbing on v3, but now I'm thinking that it
was a premature request; we either plumb in errp handling without any
new features, or we do the new features in isolation and only later see
if adding errp plumbing makes sense.  Yes, that means undoing some of
the changes you made between v3 and v4, so sorry for the churn it has
caused.

I hope to post a v5 soon with the tweaks I've made after playing with
this version.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]