[Libguestfs] [libnbd PATCH] disconnect: Prevent any further commands

[Eric Blake]

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