Libguestfs - Jun 2019 - [libnbd PATCH] disconnect: Prevent any further commands

Once the client has requested NBD_CMD_DISC, the protocol states that
it must not send any further information to the server (further writes
may still be needed for a clean TLS shutdown, but that's a different
matter requiring more states).

Our state machine can prevent some of this if we have moved to CLOSED,
but that's not foolproof because we can queue commands that can't be
written right away and thus not be in CLOSED yet. Solve this by
instead tracking when we queue a disconnect request, and rejecting all
commands after that point even while still allowing replies from the
server for existing in-flight commands.

The protocol also recommends that NBD_CMD_DISC not be sent until there
are no other pending in-flight commands, but at the moment, we place
that burden on the client.  Perhaps we should add a knob to
nbd_shutdown and/or add a new API nbd_aio_in_flight returning the
number of in-flight commands, to make things easier?
---
 lib/disconnect.c | 6 ++++--
 lib/internal.h   | 2 ++
 lib/rw.c         | 5 +++++
 tests/errors.c   | 9 ++++++---
 4 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/lib/disconnect.c b/lib/disconnect.c
index 95e9a37..53de386 100644
--- a/lib/disconnect.c
+++ b/lib/disconnect.c
@@ -29,8 +29,9 @@
 int
 nbd_unlocked_shutdown (struct nbd_handle *h)
 {
-  if (nbd_internal_is_state_ready (get_next_state (h)) ||
-      nbd_internal_is_state_processing (get_next_state (h))) {
+  if (!h->disconnect_request &&
+      (nbd_internal_is_state_ready (get_next_state (h)) ||
+       nbd_internal_is_state_processing (get_next_state (h)))) {
     if (nbd_unlocked_aio_disconnect (h, 0) == -1)
       return -1;
   }
@@ -57,6 +58,7 @@ nbd_unlocked_aio_disconnect (struct nbd_handle *h, uint32_t
flags)
   id = nbd_internal_command_common (h, 0, NBD_CMD_DISC, 0, 0, NULL, NULL);
   if (id == -1)
     return -1;
+  h->disconnect_request = true;

   /* This will leave the command on the in-flight list.  Is this a
    * problem?  Probably it isn't.  If it is, we could add a flag to
diff --git a/lib/internal.h b/lib/internal.h
index 88ad703..11e0db6 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -191,6 +191,8 @@ struct nbd_handle {
   struct command_in_flight *cmds_to_issue, *cmds_in_flight, *cmds_done;
   /* Current command during a REPLY cycle */
   struct command_in_flight *reply_cmd;
+
+  bool disconnect_request;      /* True if we've sent NBD_CMD_DISC */
 };

 struct meta_context {
diff --git a/lib/rw.c b/lib/rw.c
index 2dc60de..6b57f11 100644
--- a/lib/rw.c
+++ b/lib/rw.c
@@ -163,6 +163,11 @@ nbd_internal_command_common (struct nbd_handle *h,
 {
   struct command_in_flight *cmd, *prev_cmd;

+  if (h->disconnect_request) {
+      set_error (EINVAL, "cannot request more commands after
NBD_CMD_DISC");
+      return -1;
+  }
+
   switch (type) {
     /* Commands which send or receive data are limited to MAX_REQUEST_SIZE. */
   case NBD_CMD_READ:
diff --git a/tests/errors.c b/tests/errors.c
index 415c378..faa1488 100644
--- a/tests/errors.c
+++ b/tests/errors.c
@@ -168,7 +168,7 @@ main (int argc, char *argv[])
   check (ERANGE, "nbd_aio_pwrite: ");

   /* Queue up a write command so large that we block on POLLIN, then queue
-   * multiple disconnects. XXX The last one should fail.
+   * multiple disconnects.
    */
   if (nbd_aio_pwrite (nbd, buf, 2 * 1024 * 1024, 0, 0) == -1) {
     fprintf (stderr, "%s: %s\n", argv[0], nbd_get_error ());
@@ -184,10 +184,13 @@ main (int argc, char *argv[])
     fprintf (stderr, "%s: %s\n", argv[0], nbd_get_error ());
     exit (EXIT_FAILURE);
   }
-  if (nbd_aio_disconnect (nbd, 0) == -1) { /* XXX */
-    fprintf (stderr, "%s: %s\n", argv[0], nbd_get_error ());
+  if (nbd_aio_disconnect (nbd, 0) != -1) {
+    fprintf (stderr, "%s: test failed: "
+             "no diagnosis that nbd_aio_disconnect prevents new
commands\n",
+             argv[0]);
     exit (EXIT_FAILURE);
   }
+  check (EINVAL, "nbd_aio_disconnect: ");

   /* Flush the queue (whether this one fails is a race with how fast
    * the server shuts down, so don't enforce status), then try to send
-- 
2.20.1

On Fri, Jun 28, 2019 at 04:29:28PM -0500, Eric Blake
wrote:
> Once the client has requested NBD_CMD_DISC, the protocol states that
> it must not send any further information to the server (further writes
> may still be needed for a clean TLS shutdown, but that's a different
> matter requiring more states).
> 
> Our state machine can prevent some of this if we have moved to CLOSED,
> but that's not foolproof because we can queue commands that can't
be
> written right away and thus not be in CLOSED yet. Solve this by
> instead tracking when we queue a disconnect request, and rejecting all
> commands after that point even while still allowing replies from the
> server for existing in-flight commands.
The patch looks fine, so ACK.
> The protocol also recommends that NBD_CMD_DISC not be sent until there
> are no other pending in-flight commands, but at the moment, we place
> that burden on the client.  Perhaps we should add a knob to
> nbd_shutdown and/or add a new API nbd_aio_in_flight returning the
> number of in-flight commands, to make things easier?
I'm not clear what the client code would look like for this, if we had
this new API.  It seems like the client would need its own flag ("get
ready to disconnect") and it would need to check this flag + the
number of commands in flight every time before entering poll, and then
issue the disconnect at the right time.  Multi-conn makes this even
more complicated.

Also there's the question of what counts as a commnd in flight --
commands which are queued in the handle and not sent might be
considered different from commands which have been sent but for which
no replies have been received (although not for this particular case).

Rich.
>  lib/disconnect.c | 6 ++++--
>  lib/internal.h   | 2 ++
>  lib/rw.c         | 5 +++++
>  tests/errors.c   | 9 ++++++---
>  4 files changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/disconnect.c b/lib/disconnect.c
> index 95e9a37..53de386 100644
> --- a/lib/disconnect.c
> +++ b/lib/disconnect.c
> @@ -29,8 +29,9 @@
>  int
>  nbd_unlocked_shutdown (struct nbd_handle *h)
>  {
> -  if (nbd_internal_is_state_ready (get_next_state (h)) ||
> -      nbd_internal_is_state_processing (get_next_state (h))) {
> +  if (!h->disconnect_request &&
> +      (nbd_internal_is_state_ready (get_next_state (h)) ||
> +       nbd_internal_is_state_processing (get_next_state (h)))) {
>      if (nbd_unlocked_aio_disconnect (h, 0) == -1)
>        return -1;
>    }
> @@ -57,6 +58,7 @@ nbd_unlocked_aio_disconnect (struct nbd_handle *h,
uint32_t flags)
>    id = nbd_internal_command_common (h, 0, NBD_CMD_DISC, 0, 0, NULL, NULL);
>    if (id == -1)
>      return -1;
> +  h->disconnect_request = true;
> 
>    /* This will leave the command on the in-flight list.  Is this a
>     * problem?  Probably it isn't.  If it is, we could add a flag to
> diff --git a/lib/internal.h b/lib/internal.h
> index 88ad703..11e0db6 100644
> --- a/lib/internal.h
> +++ b/lib/internal.h
> @@ -191,6 +191,8 @@ struct nbd_handle {
>    struct command_in_flight *cmds_to_issue, *cmds_in_flight, *cmds_done;
>    /* Current command during a REPLY cycle */
>    struct command_in_flight *reply_cmd;
> +
> +  bool disconnect_request;      /* True if we've sent NBD_CMD_DISC */
>  };
> 
>  struct meta_context {
> diff --git a/lib/rw.c b/lib/rw.c
> index 2dc60de..6b57f11 100644
> --- a/lib/rw.c
> +++ b/lib/rw.c
> @@ -163,6 +163,11 @@ nbd_internal_command_common (struct nbd_handle *h,
>  {
>    struct command_in_flight *cmd, *prev_cmd;
> 
> +  if (h->disconnect_request) {
> +      set_error (EINVAL, "cannot request more commands after
NBD_CMD_DISC");
> +      return -1;
> +  }
> +
>    switch (type) {
>      /* Commands which send or receive data are limited to
MAX_REQUEST_SIZE. */
>    case NBD_CMD_READ:
> diff --git a/tests/errors.c b/tests/errors.c
> index 415c378..faa1488 100644
> --- a/tests/errors.c
> +++ b/tests/errors.c
> @@ -168,7 +168,7 @@ main (int argc, char *argv[])
>    check (ERANGE, "nbd_aio_pwrite: ");
> 
>    /* Queue up a write command so large that we block on POLLIN, then queue
> -   * multiple disconnects. XXX The last one should fail.
> +   * multiple disconnects.
>     */
>    if (nbd_aio_pwrite (nbd, buf, 2 * 1024 * 1024, 0, 0) == -1) {
>      fprintf (stderr, "%s: %s\n", argv[0], nbd_get_error ());
> @@ -184,10 +184,13 @@ main (int argc, char *argv[])
>      fprintf (stderr, "%s: %s\n", argv[0], nbd_get_error ());
>      exit (EXIT_FAILURE);
>    }
> -  if (nbd_aio_disconnect (nbd, 0) == -1) { /* XXX */
> -    fprintf (stderr, "%s: %s\n", argv[0], nbd_get_error ());
> +  if (nbd_aio_disconnect (nbd, 0) != -1) {
> +    fprintf (stderr, "%s: test failed: "
> +             "no diagnosis that nbd_aio_disconnect prevents new
commands\n",
> +             argv[0]);
>      exit (EXIT_FAILURE);
>    }
> +  check (EINVAL, "nbd_aio_disconnect: ");
> 
>    /* Flush the queue (whether this one fails is a race with how fast
>     * the server shuts down, so don't enforce status), then try to send
> -- 
> 2.20.1
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs
-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW