[Qemu-devel] [PULL 0/4] NBD patches for 2.11-rc2

[Qemu-devel] [PULL 0/4] NBD patches for 2.11-rc2

[Eric Blake]

The following changes since commit fec035a53fa15c4c8c4e62bfef56a35df4161e38:

  Merge remote-tracking branch 'remotes/kraxel/tags/ui-20171117-pull-request' into staging (2017-11-17 10:18:41 +0000)

are available in the git repository at:

  git://repo.or.cz/qemu/ericb.git tags/pull-nbd-2017-11-17

for you to fetch changes up to fed5f8f82056c9f222433c41aeb9fca50c89f297:

  nbd/server: Fix error reporting for bad requests (2017-11-17 08:38:38 -0600)

----------------------------------------------------------------
nbd patches for 2017-11-17

Eric Blake - nbd: Don't crash when server reports NBD_CMD_READ failure
Eric Blake - nbd/client: Use error_prepend() correctly
Eric Blake - nbd/client: Don't hard-disconnect on ESHUTDOWN from server
Eric Blake - nbd/server: Fix error reporting for bad requests

----------------------------------------------------------------
Eric Blake (4):
      nbd: Don't crash when server reports NBD_CMD_READ failure
      nbd/client: Use error_prepend() correctly
      nbd/client: Don't hard-disconnect on ESHUTDOWN from server
      nbd/server: Fix error reporting for bad requests

 block/nbd-client.c |  4 ++--
 nbd/client.c       | 56 +++++++++++++++++++++++++-----------------------------
 nbd/server.c       | 36 ++++++++++++-----------------------
 3 files changed, 40 insertions(+), 56 deletions(-)

-- 
2.13.6

[Qemu-devel] [PULL 1/4] nbd: Don't crash when server reports NBD_CMD_READ failure

[Eric Blake]

If a server fails a read, for example with EIO, but the connection
is still live, then we would crash trying to print a non-existent
error message in nbd_client_co_preadv().  For consistency, also
change the error printout in nbd_read_reply_entry(), although that
instance does not crash.  Bug introduced in commit f140e300.

Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <20171112013936.5942-1-eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/nbd-client.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/nbd-client.c b/block/nbd-client.c
index bcfed0133d..9206652e45 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -78,7 +78,7 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)
     while (!s->quit) {
         assert(s->reply.handle == 0);
         ret = nbd_receive_reply(s->ioc, &s->reply, &local_err);
-        if (ret < 0) {
+        if (local_err) {
             error_report_err(local_err);
         }
         if (ret <= 0) {
@@ -691,7 +691,7 @@ int nbd_client_co_preadv(BlockDriverState *bs, uint64_t offset,

     ret = nbd_co_receive_cmdread_reply(client, request.handle, offset, qiov,
                                        &local_err);
-    if (ret < 0) {
+    if (local_err) {
         error_report_err(local_err);
     }
     return ret;
-- 
2.13.6

[Qemu-devel] [PULL 2/4] nbd/client: Use error_prepend() correctly

[Eric Blake]

When using error prepend(), it is necessary to end with a space
in the format string; otherwise, messages come out incorrectly,
such as when connecting to a socket that hangs up immediately:

can't open device nbd://localhost:10809/: Failed to read dataUnexpected end-of-file before all bytes were read

Originally botched in commit e44ed99d, then several more instances
added in the meantime.

Pre-existing and not fixed here: we are inconsistent on capitalization;
some of our messages start with lower case, and others start with upper,
although the use of error_prepend() is much nicer to read when all
fragments consistently start with lower.

CC: qemu-stable@nongnu.org
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <20171113152424.25381-1-eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 nbd/client.c | 50 ++++++++++++++++++++++++++------------------------
 1 file changed, 26 insertions(+), 24 deletions(-)

diff --git a/nbd/client.c b/nbd/client.c
index 1880103d2a..4e15fc484d 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -79,12 +79,12 @@ static int nbd_send_option_request(QIOChannel *ioc, uint32_t opt,
     stl_be_p(&req.length, len);

     if (nbd_write(ioc, &req, sizeof(req), errp) < 0) {
-        error_prepend(errp, "Failed to send option request header");
+        error_prepend(errp, "Failed to send option request header: ");
         return -1;
     }

     if (len && nbd_write(ioc, (char *) data, len, errp) < 0) {
-        error_prepend(errp, "Failed to send option request data");
+        error_prepend(errp, "Failed to send option request data: ");
         return -1;
     }

@@ -113,7 +113,7 @@ static int nbd_receive_option_reply(QIOChannel *ioc, uint32_t opt,
 {
     QEMU_BUILD_BUG_ON(sizeof(*reply) != 20);
     if (nbd_read(ioc, reply, sizeof(*reply), errp) < 0) {
-        error_prepend(errp, "failed to read option reply");
+        error_prepend(errp, "failed to read option reply: ");
         nbd_send_opt_abort(ioc);
         return -1;
     }
@@ -166,7 +166,7 @@ static int nbd_handle_reply_err(QIOChannel *ioc, nbd_opt_reply *reply,
         msg = g_malloc(reply->length + 1);
         if (nbd_read(ioc, msg, reply->length, errp) < 0) {
             error_prepend(errp, "failed to read option error 0x%" PRIx32
-                          " (%s) message",
+                          " (%s) message: ",
                           reply->type, nbd_rep_lookup(reply->type));
             goto cleanup;
         }
@@ -277,7 +277,7 @@ static int nbd_receive_list(QIOChannel *ioc, const char *want, bool *match,
         return -1;
     }
     if (nbd_read(ioc, &namelen, sizeof(namelen), errp) < 0) {
-        error_prepend(errp, "failed to read option name length");
+        error_prepend(errp, "failed to read option name length: ");
         nbd_send_opt_abort(ioc);
         return -1;
     }
@@ -290,7 +290,8 @@ static int nbd_receive_list(QIOChannel *ioc, const char *want, bool *match,
     }
     if (namelen != strlen(want)) {
         if (nbd_drop(ioc, len, errp) < 0) {
-            error_prepend(errp, "failed to skip export name with wrong length");
+            error_prepend(errp,
+                          "failed to skip export name with wrong length: ");
             nbd_send_opt_abort(ioc);
             return -1;
         }
@@ -299,14 +300,14 @@ static int nbd_receive_list(QIOChannel *ioc, const char *want, bool *match,

     assert(namelen < sizeof(name));
     if (nbd_read(ioc, name, namelen, errp) < 0) {
-        error_prepend(errp, "failed to read export name");
+        error_prepend(errp, "failed to read export name: ");
         nbd_send_opt_abort(ioc);
         return -1;
     }
     name[namelen] = '\0';
     len -= namelen;
     if (nbd_drop(ioc, len, errp) < 0) {
-        error_prepend(errp, "failed to read export description");
+        error_prepend(errp, "failed to read export description: ");
         nbd_send_opt_abort(ioc);
         return -1;
     }
@@ -390,7 +391,7 @@ static int nbd_opt_go(QIOChannel *ioc, const char *wantname,
             return -1;
         }
         if (nbd_read(ioc, &type, sizeof(type), errp) < 0) {
-            error_prepend(errp, "failed to read info type");
+            error_prepend(errp, "failed to read info type: ");
             nbd_send_opt_abort(ioc);
             return -1;
         }
@@ -405,13 +406,13 @@ static int nbd_opt_go(QIOChannel *ioc, const char *wantname,
                 return -1;
             }
             if (nbd_read(ioc, &info->size, sizeof(info->size), errp) < 0) {
-                error_prepend(errp, "failed to read info size");
+                error_prepend(errp, "failed to read info size: ");
                 nbd_send_opt_abort(ioc);
                 return -1;
             }
             be64_to_cpus(&info->size);
             if (nbd_read(ioc, &info->flags, sizeof(info->flags), errp) < 0) {
-                error_prepend(errp, "failed to read info flags");
+                error_prepend(errp, "failed to read info flags: ");
                 nbd_send_opt_abort(ioc);
                 return -1;
             }
@@ -428,7 +429,7 @@ static int nbd_opt_go(QIOChannel *ioc, const char *wantname,
             }
             if (nbd_read(ioc, &info->min_block, sizeof(info->min_block),
                          errp) < 0) {
-                error_prepend(errp, "failed to read info minimum block size");
+                error_prepend(errp, "failed to read info minimum block size: ");
                 nbd_send_opt_abort(ioc);
                 return -1;
             }
@@ -441,7 +442,8 @@ static int nbd_opt_go(QIOChannel *ioc, const char *wantname,
             }
             if (nbd_read(ioc, &info->opt_block, sizeof(info->opt_block),
                          errp) < 0) {
-                error_prepend(errp, "failed to read info preferred block size");
+                error_prepend(errp,
+                              "failed to read info preferred block size: ");
                 nbd_send_opt_abort(ioc);
                 return -1;
             }
@@ -455,7 +457,7 @@ static int nbd_opt_go(QIOChannel *ioc, const char *wantname,
             }
             if (nbd_read(ioc, &info->max_block, sizeof(info->max_block),
                          errp) < 0) {
-                error_prepend(errp, "failed to read info maximum block size");
+                error_prepend(errp, "failed to read info maximum block size: ");
                 nbd_send_opt_abort(ioc);
                 return -1;
             }
@@ -467,7 +469,7 @@ static int nbd_opt_go(QIOChannel *ioc, const char *wantname,
         default:
             trace_nbd_opt_go_info_unknown(type, nbd_info_lookup(type));
             if (nbd_drop(ioc, len, errp) < 0) {
-                error_prepend(errp, "Failed to read info payload");
+                error_prepend(errp, "Failed to read info payload: ");
                 nbd_send_opt_abort(ioc);
                 return -1;
             }
@@ -618,7 +620,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name,
     }

     if (nbd_read(ioc, buf, 8, errp) < 0) {
-        error_prepend(errp, "Failed to read data");
+        error_prepend(errp, "Failed to read data: ");
         goto fail;
     }

@@ -637,7 +639,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name,
     }

     if (nbd_read(ioc, &magic, sizeof(magic), errp) < 0) {
-        error_prepend(errp, "Failed to read magic");
+        error_prepend(errp, "Failed to read magic: ");
         goto fail;
     }
     magic = be64_to_cpu(magic);
@@ -649,7 +651,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name,
         bool fixedNewStyle = false;

         if (nbd_read(ioc, &globalflags, sizeof(globalflags), errp) < 0) {
-            error_prepend(errp, "Failed to read server flags");
+            error_prepend(errp, "Failed to read server flags: ");
             goto fail;
         }
         globalflags = be16_to_cpu(globalflags);
@@ -665,7 +667,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name,
         /* client requested flags */
         clientflags = cpu_to_be32(clientflags);
         if (nbd_write(ioc, &clientflags, sizeof(clientflags), errp) < 0) {
-            error_prepend(errp, "Failed to send clientflags field");
+            error_prepend(errp, "Failed to send clientflags field: ");
             goto fail;
         }
         if (tlscreds) {
@@ -727,13 +729,13 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name,

         /* Read the response */
         if (nbd_read(ioc, &info->size, sizeof(info->size), errp) < 0) {
-            error_prepend(errp, "Failed to read export length");
+            error_prepend(errp, "Failed to read export length: ");
             goto fail;
         }
         be64_to_cpus(&info->size);

         if (nbd_read(ioc, &info->flags, sizeof(info->flags), errp) < 0) {
-            error_prepend(errp, "Failed to read export flags");
+            error_prepend(errp, "Failed to read export flags: ");
             goto fail;
         }
         be16_to_cpus(&info->flags);
@@ -750,13 +752,13 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name,
         }

         if (nbd_read(ioc, &info->size, sizeof(info->size), errp) < 0) {
-            error_prepend(errp, "Failed to read export length");
+            error_prepend(errp, "Failed to read export length: ");
             goto fail;
         }
         be64_to_cpus(&info->size);

         if (nbd_read(ioc, &oldflags, sizeof(oldflags), errp) < 0) {
-            error_prepend(errp, "Failed to read export flags");
+            error_prepend(errp, "Failed to read export flags: ");
             goto fail;
         }
         be32_to_cpus(&oldflags);
@@ -772,7 +774,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name,

     trace_nbd_receive_negotiate_size_flags(info->size, info->flags);
     if (zeroes && nbd_drop(ioc, 124, errp) < 0) {
-        error_prepend(errp, "Failed to read reserved block");
+        error_prepend(errp, "Failed to read reserved block: ");
         goto fail;
     }
     rc = 0;
-- 
2.13.6

[Qemu-devel] [PULL 3/4] nbd/client: Don't hard-disconnect on ESHUTDOWN from server

[Eric Blake]

The NBD spec says that a server may fail any transmission request
with ESHUTDOWN when it is apparent that no further request from
the client can be successfully honored.  The client is supposed
to then initiate a soft shutdown (wait for all remaining in-flight
requests to be answered, then send NBD_CMD_DISC).  However, since
qemu's server never uses ESHUTDOWN errors, this code was mostly
untested since its introduction in commit b6f5d3b5.

More recently, I learned that nbdkit as the NBD server is able to
send ESHUTDOWN errors, so I finally tested this code, and noticed
that our client was special-casing ESHUTDOWN to cause a hard
shutdown (immediate disconnect, with no NBD_CMD_DISC), but only
if the server sends this error as a simple reply.  Further
investigation found that commit d2febedb introduced a regression
where structured replies behave differently than simple replies -
but that the structured reply behavior is more in line with the
spec (even if we still lack code in nbd-client.c to properly quit
sending further requests).  So this patch reverts the portion of
b6f5d3b5 that introduced an improper hard-disconnect special-case
at the lower level, and leaves the future enhancement of a nicer
soft-disconnect at the higher level for another day.

CC: qemu-stable@nongnu.org
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <20171113194857.13933-1-eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 nbd/client.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/nbd/client.c b/nbd/client.c
index 4e15fc484d..eea236ca06 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -996,15 +996,9 @@ int nbd_receive_reply(QIOChannel *ioc, NBDReply *reply, Error **errp)
         if (ret < 0) {
             break;
         }
-
         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);
-- 
2.13.6

[Qemu-devel] [PULL 4/4] nbd/server: Fix error reporting for bad requests

[Eric Blake]

The NBD spec says an attempt to NBD_CMD_TRIM on a read-only
export should fail with EPERM, as a trim has the potential
to change disk contents, but we were relying on the block
layer to catch that for us, which might not always give the
right error (and even if it does, it does not let us pass
back a sane message for structured replies).

The NBD spec says an attempt to NBD_CMD_WRITE_ZEROES out of
bounds should fail with ENOSPC, not EINVAL.

Our check for u64 offset + u32 length wraparound up front is
pointless; nothing uses offset until after the second round
of sanity checks, and we can just as easily ensure there is
no wraparound by checking whether offset is in bounds (since
a disk size cannot exceed off_t which is 63 bits, adding a
32-bit number for a valid offset can't overflow).  Bonus:
dropping the up-front check lets us keep the connection alive
after NBD_CMD_WRITE, whereas before we would drop the
connection (of course, any client sending a packet that would
trigger the failure is already buggy, so it's also okay to
drop the connection, but better quality-of-implementation
never hurts).

Solve all of these issues by some code motion and improved
request validation.

Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <20171115213557.3548-1-eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 nbd/server.c | 36 ++++++++++++------------------------
 1 file changed, 12 insertions(+), 24 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index df771fd42f..7d6801b427 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1366,15 +1366,6 @@ static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request,
         return -EIO;
     }

-    /* Check for sanity in the parameters, part 1.  Defer as many
-     * checks as possible until after reading any NBD_CMD_WRITE
-     * payload, so we can try and keep the connection alive.  */
-    if ((request->from + request->len) < request->from) {
-        error_setg(errp,
-                   "integer overflow detected, you're probably being attacked");
-        return -EINVAL;
-    }
-
     if (request->type == NBD_CMD_READ || request->type == NBD_CMD_WRITE) {
         if (request->len > NBD_MAX_BUFFER_SIZE) {
             error_setg(errp, "len (%" PRIu32" ) is larger than max len (%u)",
@@ -1399,12 +1390,21 @@ static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request,
                                                       request->len);
     }

-    /* Sanity checks, part 2. */
-    if (request->from + request->len > client->exp->size) {
+    /* Sanity checks. */
+    if (client->exp->nbdflags & NBD_FLAG_READ_ONLY &&
+        (request->type == NBD_CMD_WRITE ||
+         request->type == NBD_CMD_WRITE_ZEROES ||
+         request->type == NBD_CMD_TRIM)) {
+        error_setg(errp, "Export is read-only");
+        return -EROFS;
+    }
+    if (request->from > client->exp->size ||
+        request->from + request->len > client->exp->size) {
         error_setg(errp, "operation past EOF; From: %" PRIu64 ", Len: %" PRIu32
                    ", Size: %" PRIu64, request->from, request->len,
                    (uint64_t)client->exp->size);
-        return request->type == NBD_CMD_WRITE ? -ENOSPC : -EINVAL;
+        return (request->type == NBD_CMD_WRITE ||
+                request->type == NBD_CMD_WRITE_ZEROES) ? -ENOSPC : -EINVAL;
     }
     valid_flags = NBD_CMD_FLAG_FUA;
     if (request->type == NBD_CMD_READ && client->structured_reply) {
@@ -1482,12 +1482,6 @@ static coroutine_fn void nbd_trip(void *opaque)

         break;
     case NBD_CMD_WRITE:
-        if (exp->nbdflags & NBD_FLAG_READ_ONLY) {
-            error_setg(&local_err, "Export is read-only");
-            ret = -EROFS;
-            break;
-        }
-
         flags = 0;
         if (request.flags & NBD_CMD_FLAG_FUA) {
             flags |= BDRV_REQ_FUA;
@@ -1500,12 +1494,6 @@ static coroutine_fn void nbd_trip(void *opaque)

         break;
     case NBD_CMD_WRITE_ZEROES:
-        if (exp->nbdflags & NBD_FLAG_READ_ONLY) {
-            error_setg(&local_err, "Export is read-only");
-            ret = -EROFS;
-            break;
-        }
-
         flags = 0;
         if (request.flags & NBD_CMD_FLAG_FUA) {
             flags |= BDRV_REQ_FUA;
-- 
2.13.6

Re: [Qemu-devel] [PULL 0/4] NBD patches for 2.11-rc2

[Peter Maydell]

On 17 November 2017 at 15:05, Eric Blake <eblake@redhat.com> wrote:
> The following changes since commit fec035a53fa15c4c8c4e62bfef56a35df4161e38:
>
>   Merge remote-tracking branch 'remotes/kraxel/tags/ui-20171117-pull-request' into staging (2017-11-17 10:18:41 +0000)
>
> are available in the git repository at:
>
>   git://repo.or.cz/qemu/ericb.git tags/pull-nbd-2017-11-17
>
> for you to fetch changes up to fed5f8f82056c9f222433c41aeb9fca50c89f297:
>
>   nbd/server: Fix error reporting for bad requests (2017-11-17 08:38:38 -0600)
>
> ----------------------------------------------------------------
> nbd patches for 2017-11-17
>
> Eric Blake - nbd: Don't crash when server reports NBD_CMD_READ failure
> Eric Blake - nbd/client: Use error_prepend() correctly
> Eric Blake - nbd/client: Don't hard-disconnect on ESHUTDOWN from server
> Eric Blake - nbd/server: Fix error reporting for bad requests
>
> ----------------------------------------------------------------

Applied, thanks.

-- PMM

Re: [Qemu-devel] [PULL 3/4] nbd/client: Don't hard-disconnect on ESHUTDOWN from server

[Michael Roth]

Quoting Eric Blake (2017-11-17 09:05:55)
> The NBD spec says that a server may fail any transmission request
> with ESHUTDOWN when it is apparent that no further request from
> the client can be successfully honored.  The client is supposed
> to then initiate a soft shutdown (wait for all remaining in-flight
> requests to be answered, then send NBD_CMD_DISC).  However, since
> qemu's server never uses ESHUTDOWN errors, this code was mostly
> untested since its introduction in commit b6f5d3b5.
> 
> More recently, I learned that nbdkit as the NBD server is able to
> send ESHUTDOWN errors, so I finally tested this code, and noticed
> that our client was special-casing ESHUTDOWN to cause a hard
> shutdown (immediate disconnect, with no NBD_CMD_DISC), but only
> if the server sends this error as a simple reply.  Further
> investigation found that commit d2febedb introduced a regression
> where structured replies behave differently than simple replies -
> but that the structured reply behavior is more in line with the
> spec (even if we still lack code in nbd-client.c to properly quit
> sending further requests).  So this patch reverts the portion of
> b6f5d3b5 that introduced an improper hard-disconnect special-case
> at the lower level, and leaves the future enhancement of a nicer
> soft-disconnect at the higher level for another day.
> 
> CC: qemu-stable@nongnu.org

Since 2.10.x lacks d2febedb, the patch would look something like:

diff --git a/nbd/client.c b/nbd/client.c
index 4caff77119..f04e95542f 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -945,12 +945,6 @@ ssize_t nbd_receive_reply(QIOChannel *ioc, NBDReply *reply, Error **errp)
     reply->handle = ldq_be_p(buf + 8);
 
     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");
-        return -EINVAL;
-    }
     trace_nbd_receive_reply(magic, reply->error, reply->handle);
 
     if (magic != NBD_REPLY_MAGIC) {

If that seems reasonable I can apply it as such.

> Signed-off-by: Eric Blake <eblake@redhat.com>
> Message-Id: <20171113194857.13933-1-eblake@redhat.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  nbd/client.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/nbd/client.c b/nbd/client.c
> index 4e15fc484d..eea236ca06 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -996,15 +996,9 @@ int nbd_receive_reply(QIOChannel *ioc, NBDReply *reply, Error **errp)
>          if (ret < 0) {
>              break;
>          }
> -
>          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);
> -- 
> 2.13.6
> 
> 

Re: [Qemu-devel] [PULL 3/4] nbd/client: Don't hard-disconnect on ESHUTDOWN from server

[Eric Blake]

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

On 12/06/2017 10:55 AM, Michael Roth wrote:
> Quoting Eric Blake (2017-11-17 09:05:55)
>> The NBD spec says that a server may fail any transmission request
>> with ESHUTDOWN when it is apparent that no further request from
>> the client can be successfully honored.  The client is supposed
>> to then initiate a soft shutdown (wait for all remaining in-flight
>> requests to be answered, then send NBD_CMD_DISC).  However, since
>> qemu's server never uses ESHUTDOWN errors, this code was mostly
>> untested since its introduction in commit b6f5d3b5.
>>
>> More recently, I learned that nbdkit as the NBD server is able to
>> send ESHUTDOWN errors, so I finally tested this code, and noticed
>> that our client was special-casing ESHUTDOWN to cause a hard
>> shutdown (immediate disconnect, with no NBD_CMD_DISC), but only
>> if the server sends this error as a simple reply.  Further
>> investigation found that commit d2febedb introduced a regression
>> where structured replies behave differently than simple replies -
>> but that the structured reply behavior is more in line with the
>> spec (even if we still lack code in nbd-client.c to properly quit
>> sending further requests).  So this patch reverts the portion of
>> b6f5d3b5 that introduced an improper hard-disconnect special-case
>> at the lower level, and leaves the future enhancement of a nicer
>> soft-disconnect at the higher level for another day.
>>
>> CC: qemu-stable@nongnu.org
> 
> Since 2.10.x lacks d2febedb, the patch would look something like:
> 
> diff --git a/nbd/client.c b/nbd/client.c
> index 4caff77119..f04e95542f 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -945,12 +945,6 @@ ssize_t nbd_receive_reply(QIOChannel *ioc, NBDReply *reply, Error **errp)
>      reply->handle = ldq_be_p(buf + 8);
>  
>      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");
> -        return -EINVAL;
> -    }
>      trace_nbd_receive_reply(magic, reply->error, reply->handle);
>  
>      if (magic != NBD_REPLY_MAGIC) {
> 
> If that seems reasonable I can apply it as such.

Your backport looks correct to me; thanks.

-- 
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 --]