Libguestfs - Sep 2020 - Re: [libnbd PATCH] api: Add LIBNBD_SHUTDOWN_IMMEDIATE flag

As mentioned in commits 176fc4ea and 609c25f0, our original plan in
adding a flags argument to nbd_shutdown was to let us specify
different behaviors at the libnbd level, rather than NBD protocol
flags (for that, the user has nbd_aio_disconnect).  But when we later
parameterized OFlags to accept various bitmasks (commit f891340b), we
failed to mark nbd_shutdown as using a different bitmask than
NBD_CMD_FLAG.

I've finally found a use for such a flag.  By itself,
nbd_aio_disconnect merely queues itself at the back of all pending
commands.  But if the server is processing responses slowly, it can be
desirable to elevate a disconnect request to the front of the queue,
intentionally abandoning all subsequent commands that have not already
started on their way to the server.

Fortunately, we have been rejecting all flag values, so changing the
type of the OFlags argument now has no impact to either the C API or
the ABI.  Other language bindings that are more strongly-typed will
see a different enum, but we don't promise compatibility there, and
even then, languages that permit omitting the flags argument in favor
of a default don't see any difference for client that were omitting
the argument in favor of the default.

Note that the new LIBNBD_SHUTDOWN_IMMEDIATE flag is not necessarily
instant: if the server is still receiving the previous command, we
have to wait for that before the server can learn of our intent, and
the command itself still blocks until we enter closed or dead states.
But it can certainly reduce the time spent in nbd_shutdown by not
having to wait for all unsent commands in the queue to also be
processed by the server.
---
 lib/internal.h         |   2 +
 generator/API.ml       |  28 ++++++-
 generator/states.c     |  12 +--
 lib/disconnect.c       |  16 +++-
 tests/Makefile.am      |   7 ++
 tests/shutdown-flags.c | 166 +++++++++++++++++++++++++++++++++++++++++
 .gitignore             |   1 +
 7 files changed, 221 insertions(+), 11 deletions(-)
 create mode 100644 tests/shutdown-flags.c

diff --git a/lib/internal.h b/lib/internal.h
index b2637bd..96699b5 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -435,6 +435,8 @@ extern int64_t nbd_internal_command_common (struct
nbd_handle *h,
 struct socket *nbd_internal_socket_create (int fd);

 /* states.c */
+extern void nbd_internal_abort_commands (struct nbd_handle *h,
+                                         struct command **list);
 extern int nbd_internal_run (struct nbd_handle *h, enum external_event ev);
 extern const char *nbd_internal_state_short_string (enum state state);
 extern enum state_group nbd_internal_state_group (enum state state);
diff --git a/generator/API.ml b/generator/API.ml
index 1d920cf..6cdab34 100644
--- a/generator/API.ml
+++ b/generator/API.ml
@@ -181,7 +181,14 @@ let allow_transport_flags = {
     "VSOCK", 1 lsl 2;
   ]
 }
-let all_flags = [ cmd_flags; handshake_flags; allow_transport_flags ]
+let shutdown_flags = {
+  flag_prefix = "SHUTDOWN";
+  flags = [
+    "IMMEDIATE", 1 lsl 1;
+  ]
+}
+let all_flags = [ cmd_flags; handshake_flags; allow_transport_flags;
+                  shutdown_flags]

 let default_call = { args = []; optargs = []; ret = RErr;
                      shortdesc = ""; longdesc = ""; example
= None;
@@ -1595,7 +1602,7 @@ L<nbd_can_fua(3)>).";

   "shutdown", {
     default_call with
-    args = []; optargs = [ OFlags ("flags", cmd_flags) ]; ret = RErr;
+    args = []; optargs = [ OFlags ("flags", shutdown_flags) ]; ret =
RErr;
     permitted_states = [ Connected ];
     shortdesc = "disconnect from the NBD server";
     longdesc = "\
@@ -1608,8 +1615,21 @@ This function works whether or not the handle is ready
for
 transmission of commands. If more fine-grained control is
 needed, see L<nbd_aio_disconnect(3)>.

-The C<flags> parameter must be C<0> for now (it exists for future
NBD
-protocol extensions).";
+The C<flags> argument is a bitmask, including zero or more of the
+following shutdown flags:
+
+=over 4
+
+=item C<LIBNBD_SHUTDOWN_IMMEDIATE> = 1
+
+If there are any pending requests which have not yet been sent to
+the server (see L<nbd_aio_in_flight(3)>), abandon them without
+sending them to the server, rather than the usual practice of
+issuing those commands before informing the server of the intent
+to disconnect.
+
+=back
+";
     see_also = [Link "close"; Link "aio_disconnect"];
     example = Some "examples/reads-and-writes.c";
   };
diff --git a/generator/states.c b/generator/states.c
index 9a12e82..0ecba7e 100644
--- a/generator/states.c
+++ b/generator/states.c
@@ -122,8 +122,8 @@ abort_option (struct nbd_handle *h)
 }

 /* Forcefully fail any remaining in-flight commands in list */
-static void
-abort_commands (struct nbd_handle *h, struct command **list)
+void
+nbd_internal_abort_commands (struct nbd_handle *h, struct command **list)
 {
   struct command *next, *cmd;

@@ -179,8 +179,8 @@ STATE_MACHINE {
   /* The caller should have used set_error() before reaching here */
   assert (nbd_get_error ());
   abort_option (h);
-  abort_commands (h, &h->cmds_to_issue);
-  abort_commands (h, &h->cmds_in_flight);
+  nbd_internal_abort_commands (h, &h->cmds_to_issue);
+  nbd_internal_abort_commands (h, &h->cmds_in_flight);
   h->in_flight = 0;
   if (h->sock) {
     h->sock->ops->close (h->sock);
@@ -190,8 +190,8 @@ STATE_MACHINE {

  CLOSED:
   abort_option (h);
-  abort_commands (h, &h->cmds_to_issue);
-  abort_commands (h, &h->cmds_in_flight);
+  nbd_internal_abort_commands (h, &h->cmds_to_issue);
+  nbd_internal_abort_commands (h, &h->cmds_in_flight);
   h->in_flight = 0;
   if (h->sock) {
     h->sock->ops->close (h->sock);
diff --git a/lib/disconnect.c b/lib/disconnect.c
index dcb95d8..b8356b7 100644
--- a/lib/disconnect.c
+++ b/lib/disconnect.c
@@ -23,17 +23,31 @@
 #include <stdbool.h>
 #include <errno.h>
 #include <inttypes.h>
+#include <assert.h>

 #include "internal.h"

 int
 nbd_unlocked_shutdown (struct nbd_handle *h, uint32_t flags)
 {
-  if (flags != 0) {
+  if ((flags & ~LIBNBD_SHUTDOWN_IMMEDIATE) != 0) {
     set_error (EINVAL, "invalid flag: %" PRIu32, flags);
     return -1;
   }

+  /* If IMMEDIATE, abort any commands that have not yet had any bytes
+   * sent to the server, so that NBD_CMD_DISC will be first in line.
+   */
+  if (flags & LIBNBD_SHUTDOWN_IMMEDIATE) {
+    struct command **cmd = &h->cmds_to_issue;
+    if (!nbd_internal_is_state_ready (get_next_state (h))) {
+      assert (*cmd);
+      h->cmds_to_issue_tail = *cmd;
+      cmd = &((*cmd)->next);
+    }
+    nbd_internal_abort_commands (h, cmd);
+  }
+
   if (!h->disconnect_request &&
       (nbd_internal_is_state_ready (get_next_state (h)) ||
        nbd_internal_is_state_processing (get_next_state (h)))) {
diff --git a/tests/Makefile.am b/tests/Makefile.am
index be7c83c..007c69e 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -142,6 +142,7 @@ if HAVE_NBDKIT
 check_PROGRAMS += \
 	errors \
 	server-death \
+	shutdown-flags \
 	get-size \
 	read-only-flag \
 	read-write-flag \
@@ -180,6 +181,7 @@ check_PROGRAMS += \
 TESTS += \
 	errors \
 	server-death \
+	shutdown-flags \
 	get-size \
 	read-only-flag \
 	read-write-flag \
@@ -225,6 +227,11 @@ server_death_CPPFLAGS = -I$(top_srcdir)/include
 server_death_CFLAGS = $(WARNINGS_CFLAGS)
 server_death_LDADD = $(top_builddir)/lib/libnbd.la

+shutdown_flags_SOURCES = shutdown-flags.c
+shutdown_flags_CPPFLAGS = -I$(top_srcdir)/include
+shutdown_flags_CFLAGS = $(WARNINGS_CFLAGS)
+shutdown_flags_LDADD = $(top_builddir)/lib/libnbd.la
+
 get_size_SOURCES = get-size.c
 get_size_CPPFLAGS = -I$(top_srcdir)/include
 get_size_CFLAGS = $(WARNINGS_CFLAGS)
diff --git a/tests/shutdown-flags.c b/tests/shutdown-flags.c
new file mode 100644
index 0000000..6043ee2
--- /dev/null
+++ b/tests/shutdown-flags.c
@@ -0,0 +1,166 @@
+/* NBD client library in userspace
+ * Copyright (C) 2013-2020 Red Hat Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+/* Deliberately shutdown while stranding commands, to check their status.
+ */
+
+#include <config.h>
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <errno.h>
+#include <signal.h>
+
+#include <libnbd.h>
+
+static bool write_retired;
+static const char *progname;
+
+static int
+callback (void *ignored, int *error)
+{
+  if (*error != ENOTCONN) {
+    fprintf (stderr, "%s: unexpected error in pwrite callback: %s\n",
+             progname, strerror (*error));
+    return 0;
+  }
+  write_retired = 1;
+  return 1;
+}
+
+static char buf[2 * 1024 * 1024];
+
+int
+main (int argc, char *argv[])
+{
+  struct nbd_handle *nbd;
+  int err;
+  const char *msg;
+  int64_t cookie;
+  const char *cmd[] = { "nbdkit", "-s",
"--exit-with-parent",
+                        "memory", "size=2m", NULL };
+
+  progname = argv[0];
+
+  nbd = nbd_create ();
+  if (nbd == NULL) {
+    fprintf (stderr, "%s: %s\n", argv[0], nbd_get_error ());
+    exit (EXIT_FAILURE);
+  }
+
+  /* Connect to a server. */
+  if (nbd_connect_command (nbd, (char **) cmd) == -1) {
+    fprintf (stderr, "%s: %s\n", argv[0], nbd_get_error ());
+    exit (EXIT_FAILURE);
+  }
+
+  /* Pause the server from reading, so that our first request will
+   * exceed the buffer and force the second request to be stuck client
+   * side (without stopping the server, we would be racing on whether
+   * we hit a block on writes based on whether the server can read
+   * faster than we can fill the pipe).
+   */
+  if (nbd_kill_subprocess (nbd, SIGSTOP) == -1) {
+    fprintf (stderr, "%s: test failed: nbd_kill_subprocess: %s\n",
argv[0],
+             nbd_get_error ());
+    exit (EXIT_FAILURE);
+  }
+
+  /* Issue back-to-back write requests, both large enough to block.  Set up
+   * the second to auto-retire via callback.
+   */
+  if ((cookie = nbd_aio_pwrite (nbd, buf, sizeof buf, 0,
+                                NBD_NULL_COMPLETION, 0)) == -1) {
+    fprintf (stderr, "%s: test failed: first nbd_aio_pwrite: %s\n",
argv[0],
+             nbd_get_error ());
+    exit (EXIT_FAILURE);
+  }
+  if (nbd_aio_pwrite (nbd, buf, sizeof buf, 0,
+                      (nbd_completion_callback) { .callback = callback },
+                      0) == -1) {
+    fprintf (stderr, "%s: test failed: second nbd_aio_pwrite: %s\n",
argv[0],
+             nbd_get_error ());
+    exit (EXIT_FAILURE);
+  }
+
+  /* Resume the server; but now our state machine remains blocked
+   * until we notify or otherwise poll it.
+   */
+  if (nbd_kill_subprocess (nbd, SIGCONT) == -1) {
+    fprintf (stderr, "%s: test failed: nbd_kill_subprocess: %s\n",
argv[0],
+             nbd_get_error ());
+    exit (EXIT_FAILURE);
+  }
+  if (nbd_aio_peek_command_completed (nbd) != 0) {
+    fprintf (stderr, "%s: test failed:
nbd_aio_peek_command_completed\n",
+             argv[0]);
+    exit (EXIT_FAILURE);
+  }
+  if (nbd_aio_command_completed (nbd, cookie) != 0) {
+    fprintf (stderr, "%s: test failed: nbd_aio_command_completed\n",
argv[0]);
+    exit (EXIT_FAILURE);
+  }
+
+  /* Send an immediate shutdown.  This will abort the second write, as
+   * well as kick the state machine to finish the first.
+   */
+  if (nbd_shutdown (nbd, LIBNBD_SHUTDOWN_IMMEDIATE) == -1) {
+    fprintf (stderr, "%s: test failed: nbd_shutdown\n", argv[0]);
+    exit (EXIT_FAILURE);
+  }
+
+  /* All in-flight commands should now be completed.  But whether the
+   * first write succeeded or failed depends on the server, so we
+   * merely retire it without checking status.
+   */
+  if (nbd_aio_in_flight (nbd) != 0) {
+    fprintf (stderr, "%s: test failed: nbd_aio_in_flight\n",
argv[0]);
+    exit (EXIT_FAILURE);
+  }
+  if (nbd_aio_peek_command_completed (nbd) != cookie) {
+    fprintf (stderr, "%s: test failed:
nbd_aio_peek_command_completed\n",
+             argv[0]);
+    exit (EXIT_FAILURE);
+  }
+  nbd_aio_command_completed (nbd, cookie);
+
+  /* With all commands retired, no further command should be pending */
+  if (!write_retired) {
+    fprintf (stderr, "%s: test failed: second nbd_aio_pwrite not
retired\n",
+             argv[0]);
+    exit (EXIT_FAILURE);
+  }
+  if (nbd_aio_peek_command_completed (nbd) != -1) {
+    fprintf (stderr, "%s: test failed:
nbd_aio_peek_command_completed\n",
+             argv[0]);
+    exit (EXIT_FAILURE);
+  }
+  msg = nbd_get_error ();
+  err = nbd_get_errno ();
+  printf ("error: \"%s\"\n", msg);
+  printf ("errno: %d (%s)\n", err, strerror (err));
+  if (err != EINVAL) {
+    fprintf (stderr, "%s: test failed: unexpected errno %d (%s)\n",
argv[0],
+             err, strerror (err));
+    exit (EXIT_FAILURE);
+  }
+
+  nbd_close (nbd);
+  exit (EXIT_SUCCESS);
+}
diff --git a/.gitignore b/.gitignore
index 6812fe8..aa9e0bb 100644
--- a/.gitignore
+++ b/.gitignore
@@ -179,6 +179,7 @@ Makefile.in
 /tests/read-only-flag
 /tests/read-write-flag
 /tests/server-death
+/tests/shutdown-flags
 /tests/synch-parallel
 /tests/synch-parallel-tls
 /tests/version
-- 
2.28.0

On 9/11/20 9:31 AM, Eric Blake wrote:
> As mentioned in commits 176fc4ea and 609c25f0, our original plan in
> adding a flags argument to nbd_shutdown was to let us specify
> different behaviors at the libnbd level, rather than NBD protocol
> flags (for that, the user has nbd_aio_disconnect).  But when we later
> parameterized OFlags to accept various bitmasks (commit f891340b), we
> failed to mark nbd_shutdown as using a different bitmask than
> NBD_CMD_FLAG.
> 
> I've finally found a use for such a flag.  By itself,
> nbd_aio_disconnect merely queues itself at the back of all pending
> commands.  But if the server is processing responses slowly, it can be
> desirable to elevate a disconnect request to the front of the queue,
> intentionally abandoning all subsequent commands that have not already
> started on their way to the server.
> 
> Fortunately, we have been rejecting all flag values, so changing the
> type of the OFlags argument now has no impact to either the C API or
> the ABI.  Other language bindings that are more strongly-typed will
> see a different enum, but we don't promise compatibility there, and
> even then, languages that permit omitting the flags argument in favor
> of a default don't see any difference for client that were omitting
> the argument in favor of the default.
> 
> Note that the new LIBNBD_SHUTDOWN_IMMEDIATE flag is not necessarily
> instant: if the server is still receiving the previous command, we
> have to wait for that before the server can learn of our intent, and
> the command itself still blocks until we enter closed or dead states.
> But it can certainly reduce the time spent in nbd_shutdown by not
> having to wait for all unsent commands in the queue to also be
> processed by the server.
An alternative to a new enum type is noting that the NBD protocol only 
has 16 bits for NBD_CMD_FLAG, but we have a uint32_t for our flags 
parameter.  We could declare that LIBNBD_CMD_FLAG_EXT_SHUTDOWN_IMMEDIATE 
has value 0x10000 (or however we name things to draw a distinction 
between flags passed over the wire vs. flags consumed only by libnbd), 
where the new extension flag is valid for nbd_shutdown but has no effect 
over the wire, and that would still leave the door open to pass through 
any actual 16-bit flag that the protocol ever adds for NBD_CMD_DISC.
> +++ b/generator/API.ml
> @@ -1595,7 +1602,7 @@ L<nbd_can_fua(3)>).";
> 
>     "shutdown", {
>       default_call with
> -    args = []; optargs = [ OFlags ("flags", cmd_flags) ]; ret =
RErr;
> +    args = []; optargs = [ OFlags ("flags", shutdown_flags) ];
ret = RErr;
>       permitted_states = [ Connected ];
>       shortdesc = "disconnect from the NBD server";
>       longdesc = "\

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

On Fri, Sep 11, 2020 at 09:31:11AM -0500, Eric Blake
wrote:
> As mentioned in commits 176fc4ea and 609c25f0, our original plan in
> adding a flags argument to nbd_shutdown was to let us specify
> different behaviors at the libnbd level, rather than NBD protocol
> flags (for that, the user has nbd_aio_disconnect).  But when we later
> parameterized OFlags to accept various bitmasks (commit f891340b), we
> failed to mark nbd_shutdown as using a different bitmask than
> NBD_CMD_FLAG.
>
> I've finally found a use for such a flag.  By itself,
> nbd_aio_disconnect merely queues itself at the back of all pending
> commands.  But if the server is processing responses slowly, it can be
> desirable to elevate a disconnect request to the front of the queue,
> intentionally abandoning all subsequent commands that have not already
> started on their way to the server.
> 
> Fortunately, we have been rejecting all flag values, so changing the
> type of the OFlags argument now has no impact to either the C API or
> the ABI.  Other language bindings that are more strongly-typed will
> see a different enum, but we don't promise compatibility there, and
> even then, languages that permit omitting the flags argument in favor
> of a default don't see any difference for client that were omitting
> the argument in favor of the default.
I think for the reasons you've outlined here it's fine to
change this from cmd_flags to shutdown_flags.  And the new
flag looks useful.

Patch looks good, so ACK.

Thanks,

Rich.
> Note that the new LIBNBD_SHUTDOWN_IMMEDIATE flag is not necessarily
> instant: if the server is still receiving the previous command, we
> have to wait for that before the server can learn of our intent, and
> the command itself still blocks until we enter closed or dead states.
> But it can certainly reduce the time spent in nbd_shutdown by not
> having to wait for all unsent commands in the queue to also be
> processed by the server.
> ---
>  lib/internal.h         |   2 +
>  generator/API.ml       |  28 ++++++-
>  generator/states.c     |  12 +--
>  lib/disconnect.c       |  16 +++-
>  tests/Makefile.am      |   7 ++
>  tests/shutdown-flags.c | 166 +++++++++++++++++++++++++++++++++++++++++
>  .gitignore             |   1 +
>  7 files changed, 221 insertions(+), 11 deletions(-)
>  create mode 100644 tests/shutdown-flags.c
> 
> diff --git a/lib/internal.h b/lib/internal.h
> index b2637bd..96699b5 100644
> --- a/lib/internal.h
> +++ b/lib/internal.h
> @@ -435,6 +435,8 @@ extern int64_t nbd_internal_command_common (struct
nbd_handle *h,
>  struct socket *nbd_internal_socket_create (int fd);
> 
>  /* states.c */
> +extern void nbd_internal_abort_commands (struct nbd_handle *h,
> +                                         struct command **list);
>  extern int nbd_internal_run (struct nbd_handle *h, enum external_event
ev);
>  extern const char *nbd_internal_state_short_string (enum state state);
>  extern enum state_group nbd_internal_state_group (enum state state);
> diff --git a/generator/API.ml b/generator/API.ml
> index 1d920cf..6cdab34 100644
> --- a/generator/API.ml
> +++ b/generator/API.ml
> @@ -181,7 +181,14 @@ let allow_transport_flags = {
>      "VSOCK", 1 lsl 2;
>    ]
>  }
> -let all_flags = [ cmd_flags; handshake_flags; allow_transport_flags ]
> +let shutdown_flags = {
> +  flag_prefix = "SHUTDOWN";
> +  flags = [
> +    "IMMEDIATE", 1 lsl 1;
> +  ]
> +}
> +let all_flags = [ cmd_flags; handshake_flags; allow_transport_flags;
> +                  shutdown_flags]
> 
>  let default_call = { args = []; optargs = []; ret = RErr;
>                       shortdesc = ""; longdesc = "";
example = None;
> @@ -1595,7 +1602,7 @@ L<nbd_can_fua(3)>).";
> 
>    "shutdown", {
>      default_call with
> -    args = []; optargs = [ OFlags ("flags", cmd_flags) ]; ret =
RErr;
> +    args = []; optargs = [ OFlags ("flags", shutdown_flags) ];
ret = RErr;
>      permitted_states = [ Connected ];
>      shortdesc = "disconnect from the NBD server";
>      longdesc = "\
> @@ -1608,8 +1615,21 @@ This function works whether or not the handle is
ready for
>  transmission of commands. If more fine-grained control is
>  needed, see L<nbd_aio_disconnect(3)>.
> 
> -The C<flags> parameter must be C<0> for now (it exists for
future NBD
> -protocol extensions).";
> +The C<flags> argument is a bitmask, including zero or more of the
> +following shutdown flags:
> +
> +=over 4
> +
> +=item C<LIBNBD_SHUTDOWN_IMMEDIATE> = 1
> +
> +If there are any pending requests which have not yet been sent to
> +the server (see L<nbd_aio_in_flight(3)>), abandon them without
> +sending them to the server, rather than the usual practice of
> +issuing those commands before informing the server of the intent
> +to disconnect.
> +
> +=back
> +";
>      see_also = [Link "close"; Link "aio_disconnect"];
>      example = Some "examples/reads-and-writes.c";
>    };
> diff --git a/generator/states.c b/generator/states.c
> index 9a12e82..0ecba7e 100644
> --- a/generator/states.c
> +++ b/generator/states.c
> @@ -122,8 +122,8 @@ abort_option (struct nbd_handle *h)
>  }
> 
>  /* Forcefully fail any remaining in-flight commands in list */
> -static void
> -abort_commands (struct nbd_handle *h, struct command **list)
> +void
> +nbd_internal_abort_commands (struct nbd_handle *h, struct command **list)
>  {
>    struct command *next, *cmd;
> 
> @@ -179,8 +179,8 @@ STATE_MACHINE {
>    /* The caller should have used set_error() before reaching here */
>    assert (nbd_get_error ());
>    abort_option (h);
> -  abort_commands (h, &h->cmds_to_issue);
> -  abort_commands (h, &h->cmds_in_flight);
> +  nbd_internal_abort_commands (h, &h->cmds_to_issue);
> +  nbd_internal_abort_commands (h, &h->cmds_in_flight);
>    h->in_flight = 0;
>    if (h->sock) {
>      h->sock->ops->close (h->sock);
> @@ -190,8 +190,8 @@ STATE_MACHINE {
> 
>   CLOSED:
>    abort_option (h);
> -  abort_commands (h, &h->cmds_to_issue);
> -  abort_commands (h, &h->cmds_in_flight);
> +  nbd_internal_abort_commands (h, &h->cmds_to_issue);
> +  nbd_internal_abort_commands (h, &h->cmds_in_flight);
>    h->in_flight = 0;
>    if (h->sock) {
>      h->sock->ops->close (h->sock);
> diff --git a/lib/disconnect.c b/lib/disconnect.c
> index dcb95d8..b8356b7 100644
> --- a/lib/disconnect.c
> +++ b/lib/disconnect.c
> @@ -23,17 +23,31 @@
>  #include <stdbool.h>
>  #include <errno.h>
>  #include <inttypes.h>
> +#include <assert.h>
> 
>  #include "internal.h"
> 
>  int
>  nbd_unlocked_shutdown (struct nbd_handle *h, uint32_t flags)
>  {
> -  if (flags != 0) {
> +  if ((flags & ~LIBNBD_SHUTDOWN_IMMEDIATE) != 0) {
>      set_error (EINVAL, "invalid flag: %" PRIu32, flags);
>      return -1;
>    }
> 
> +  /* If IMMEDIATE, abort any commands that have not yet had any bytes
> +   * sent to the server, so that NBD_CMD_DISC will be first in line.
> +   */
> +  if (flags & LIBNBD_SHUTDOWN_IMMEDIATE) {
> +    struct command **cmd = &h->cmds_to_issue;
> +    if (!nbd_internal_is_state_ready (get_next_state (h))) {
> +      assert (*cmd);
> +      h->cmds_to_issue_tail = *cmd;
> +      cmd = &((*cmd)->next);
> +    }
> +    nbd_internal_abort_commands (h, cmd);
> +  }
> +
>    if (!h->disconnect_request &&
>        (nbd_internal_is_state_ready (get_next_state (h)) ||
>         nbd_internal_is_state_processing (get_next_state (h)))) {
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index be7c83c..007c69e 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -142,6 +142,7 @@ if HAVE_NBDKIT
>  check_PROGRAMS += \
>  	errors \
>  	server-death \
> +	shutdown-flags \
>  	get-size \
>  	read-only-flag \
>  	read-write-flag \
> @@ -180,6 +181,7 @@ check_PROGRAMS += \
>  TESTS += \
>  	errors \
>  	server-death \
> +	shutdown-flags \
>  	get-size \
>  	read-only-flag \
>  	read-write-flag \
> @@ -225,6 +227,11 @@ server_death_CPPFLAGS = -I$(top_srcdir)/include
>  server_death_CFLAGS = $(WARNINGS_CFLAGS)
>  server_death_LDADD = $(top_builddir)/lib/libnbd.la
> 
> +shutdown_flags_SOURCES = shutdown-flags.c
> +shutdown_flags_CPPFLAGS = -I$(top_srcdir)/include
> +shutdown_flags_CFLAGS = $(WARNINGS_CFLAGS)
> +shutdown_flags_LDADD = $(top_builddir)/lib/libnbd.la
> +
>  get_size_SOURCES = get-size.c
>  get_size_CPPFLAGS = -I$(top_srcdir)/include
>  get_size_CFLAGS = $(WARNINGS_CFLAGS)
> diff --git a/tests/shutdown-flags.c b/tests/shutdown-flags.c
> new file mode 100644
> index 0000000..6043ee2
> --- /dev/null
> +++ b/tests/shutdown-flags.c
> @@ -0,0 +1,166 @@
> +/* NBD client library in userspace
> + * Copyright (C) 2013-2020 Red Hat Inc.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
02110-1301 USA
> + */
> +
> +/* Deliberately shutdown while stranding commands, to check their status.
> + */
> +
> +#include <config.h>
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <errno.h>
> +#include <signal.h>
> +
> +#include <libnbd.h>
> +
> +static bool write_retired;
> +static const char *progname;
> +
> +static int
> +callback (void *ignored, int *error)
> +{
> +  if (*error != ENOTCONN) {
> +    fprintf (stderr, "%s: unexpected error in pwrite callback:
%s\n",
> +             progname, strerror (*error));
> +    return 0;
> +  }
> +  write_retired = 1;
> +  return 1;
> +}
> +
> +static char buf[2 * 1024 * 1024];
> +
> +int
> +main (int argc, char *argv[])
> +{
> +  struct nbd_handle *nbd;
> +  int err;
> +  const char *msg;
> +  int64_t cookie;
> +  const char *cmd[] = { "nbdkit", "-s",
"--exit-with-parent",
> +                        "memory", "size=2m", NULL };
> +
> +  progname = argv[0];
> +
> +  nbd = nbd_create ();
> +  if (nbd == NULL) {
> +    fprintf (stderr, "%s: %s\n", argv[0], nbd_get_error ());
> +    exit (EXIT_FAILURE);
> +  }
> +
> +  /* Connect to a server. */
> +  if (nbd_connect_command (nbd, (char **) cmd) == -1) {
> +    fprintf (stderr, "%s: %s\n", argv[0], nbd_get_error ());
> +    exit (EXIT_FAILURE);
> +  }
> +
> +  /* Pause the server from reading, so that our first request will
> +   * exceed the buffer and force the second request to be stuck client
> +   * side (without stopping the server, we would be racing on whether
> +   * we hit a block on writes based on whether the server can read
> +   * faster than we can fill the pipe).
> +   */
> +  if (nbd_kill_subprocess (nbd, SIGSTOP) == -1) {
> +    fprintf (stderr, "%s: test failed: nbd_kill_subprocess:
%s\n", argv[0],
> +             nbd_get_error ());
> +    exit (EXIT_FAILURE);
> +  }
> +
> +  /* Issue back-to-back write requests, both large enough to block.  Set
up
> +   * the second to auto-retire via callback.
> +   */
> +  if ((cookie = nbd_aio_pwrite (nbd, buf, sizeof buf, 0,
> +                                NBD_NULL_COMPLETION, 0)) == -1) {
> +    fprintf (stderr, "%s: test failed: first nbd_aio_pwrite:
%s\n", argv[0],
> +             nbd_get_error ());
> +    exit (EXIT_FAILURE);
> +  }
> +  if (nbd_aio_pwrite (nbd, buf, sizeof buf, 0,
> +                      (nbd_completion_callback) { .callback = callback },
> +                      0) == -1) {
> +    fprintf (stderr, "%s: test failed: second nbd_aio_pwrite:
%s\n", argv[0],
> +             nbd_get_error ());
> +    exit (EXIT_FAILURE);
> +  }
> +
> +  /* Resume the server; but now our state machine remains blocked
> +   * until we notify or otherwise poll it.
> +   */
> +  if (nbd_kill_subprocess (nbd, SIGCONT) == -1) {
> +    fprintf (stderr, "%s: test failed: nbd_kill_subprocess:
%s\n", argv[0],
> +             nbd_get_error ());
> +    exit (EXIT_FAILURE);
> +  }
> +  if (nbd_aio_peek_command_completed (nbd) != 0) {
> +    fprintf (stderr, "%s: test failed:
nbd_aio_peek_command_completed\n",
> +             argv[0]);
> +    exit (EXIT_FAILURE);
> +  }
> +  if (nbd_aio_command_completed (nbd, cookie) != 0) {
> +    fprintf (stderr, "%s: test failed:
nbd_aio_command_completed\n", argv[0]);
> +    exit (EXIT_FAILURE);
> +  }
> +
> +  /* Send an immediate shutdown.  This will abort the second write, as
> +   * well as kick the state machine to finish the first.
> +   */
> +  if (nbd_shutdown (nbd, LIBNBD_SHUTDOWN_IMMEDIATE) == -1) {
> +    fprintf (stderr, "%s: test failed: nbd_shutdown\n",
argv[0]);
> +    exit (EXIT_FAILURE);
> +  }
> +
> +  /* All in-flight commands should now be completed.  But whether the
> +   * first write succeeded or failed depends on the server, so we
> +   * merely retire it without checking status.
> +   */
> +  if (nbd_aio_in_flight (nbd) != 0) {
> +    fprintf (stderr, "%s: test failed: nbd_aio_in_flight\n",
argv[0]);
> +    exit (EXIT_FAILURE);
> +  }
> +  if (nbd_aio_peek_command_completed (nbd) != cookie) {
> +    fprintf (stderr, "%s: test failed:
nbd_aio_peek_command_completed\n",
> +             argv[0]);
> +    exit (EXIT_FAILURE);
> +  }
> +  nbd_aio_command_completed (nbd, cookie);
> +
> +  /* With all commands retired, no further command should be pending */
> +  if (!write_retired) {
> +    fprintf (stderr, "%s: test failed: second nbd_aio_pwrite not
retired\n",
> +             argv[0]);
> +    exit (EXIT_FAILURE);
> +  }
> +  if (nbd_aio_peek_command_completed (nbd) != -1) {
> +    fprintf (stderr, "%s: test failed:
nbd_aio_peek_command_completed\n",
> +             argv[0]);
> +    exit (EXIT_FAILURE);
> +  }
> +  msg = nbd_get_error ();
> +  err = nbd_get_errno ();
> +  printf ("error: \"%s\"\n", msg);
> +  printf ("errno: %d (%s)\n", err, strerror (err));
> +  if (err != EINVAL) {
> +    fprintf (stderr, "%s: test failed: unexpected errno %d
(%s)\n", argv[0],
> +             err, strerror (err));
> +    exit (EXIT_FAILURE);
> +  }
> +
> +  nbd_close (nbd);
> +  exit (EXIT_SUCCESS);
> +}
> diff --git a/.gitignore b/.gitignore
> index 6812fe8..aa9e0bb 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -179,6 +179,7 @@ Makefile.in
>  /tests/read-only-flag
>  /tests/read-write-flag
>  /tests/server-death
> +/tests/shutdown-flags
>  /tests/synch-parallel
>  /tests/synch-parallel-tls
>  /tests/version
> -- 
> 2.28.0
> 
> _______________________________________________
> 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

On 9/17/20 8:22 AM, Richard W.M. Jones wrote:
> On Fri, Sep 11, 2020 at 09:31:11AM -0500, Eric Blake wrote:
>> As mentioned in commits 176fc4ea and 609c25f0, our original plan in
>> adding a flags argument to nbd_shutdown was to let us specify
>> different behaviors at the libnbd level, rather than NBD protocol
>> flags (for that, the user has nbd_aio_disconnect).  But when we later
>> parameterized OFlags to accept various bitmasks (commit f891340b), we
>> failed to mark nbd_shutdown as using a different bitmask than
>> NBD_CMD_FLAG.
>>
>> I've finally found a use for such a flag.  By itself,
>> nbd_aio_disconnect merely queues itself at the back of all pending
>> commands.  But if the server is processing responses slowly, it can be
>> desirable to elevate a disconnect request to the front of the queue,
>> intentionally abandoning all subsequent commands that have not already
>> started on their way to the server.
>>
>> Fortunately, we have been rejecting all flag values, so changing the
>> type of the OFlags argument now has no impact to either the C API or
>> the ABI.  Other language bindings that are more strongly-typed will
>> see a different enum, but we don't promise compatibility there, and
>> even then, languages that permit omitting the flags argument in favor
>> of a default don't see any difference for client that were omitting
>> the argument in favor of the default.
> 
> I think for the reasons you've outlined here it's fine to
> change this from cmd_flags to shutdown_flags.  And the new
> flag looks useful.
> 
> Patch looks good, so ACK.
I'm leaning towards having the flag value be 0x10000 (that is, outside 
the range of cmd flags).  Once we release with a value, it becomes 
locked into the API; but by intentionally picking something other than 
0x1, we still leave room for ABI to take command flags at a later date, 
with a little less hassle than having to translate from 
LIBNBD_SHUTDOWN_FOO to LIBNBD_CMD_FLAG_FOO.

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