When nbd_set_opt_mode() was introduced, we did not adjust the behavior of nbd_shutdown(), so as a result it fails if attempted after the initial connection but before nbd_opt_abort() or nbd_opt_go(). To avoid spurious error messages, at least nbdinfo has to perform special-casing on whether it is in opt mode (call nbd_opt_abort) or connected (call nbd_shutdown) prior to calling nbd_close. It would be nicer to just have a single API perform whatever is needed to gracefully shut the connection, without having to hard-code the special casing into each application. The approach taken here is to add a new flag; if the flag is absent (behavior of an older application), then nbd_opt_abort must still be invoked separately before attempting nbd_shutdown. But if the flag is present, then whether or not the server supports opt mode, the shutdown will be graceful. This patch updates the unit test to show the effect of the new flag, and the next commit will simplify nbdinfo by utilizing it. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- generator/API.ml | 21 ++++++++++++++++----- lib/disconnect.c | 15 +++++++++++++++ tests/shutdown-opt-mode.c | 27 ++++++++++++++++++++++++++- 3 files changed, 57 insertions(+), 6 deletions(-) diff --git a/generator/API.ml b/generator/API.ml index 1a9c8141..0b837259 100644 --- a/generator/API.ml +++ b/generator/API.ml @@ -255,6 +255,7 @@ let shutdown_flags = flag_prefix = "SHUTDOWN"; flags = [ "ABANDON_PENDING", 1 lsl 16; + "COVER_OPT_MODE", 1 lsl 17; ] } let all_flags = [ cmd_flags; handshake_flags; strict_flags; @@ -1231,7 +1232,8 @@ "set_opt_mode", { starting the connection. To leave the mode and proceed on to the ready state, you must use L<nbd_opt_go(3)> successfully; a failed L<nbd_opt_go(3)> returns to the negotiating state to allow a change of -export name before trying again. You may also use L<nbd_opt_abort(3)> +export name before trying again. You may also use L<nbd_opt_abort(3)>, +or the C<LIBNBD_SHUTDOWN_COVER_OPT_MODE> flag of L<nbd_shutdown(3)>, to end the connection without finishing negotiation."; example = Some "examples/list-exports.c"; see_also = [Link "get_opt_mode"; Link "aio_is_negotiating"; @@ -1240,7 +1242,7 @@ "set_opt_mode", { Link "opt_set_meta_context"; Link "opt_starttls"; Link "opt_structured_reply"; Link "set_tls"; Link "set_request_structured_replies"; - Link "aio_connect"]; + Link "aio_connect"; Link "shutdown"]; }; "get_opt_mode", { @@ -2667,7 +2669,7 @@ "shutdown", { default_call with args = []; optargs = [ OFlags ("flags", shutdown_flags, None) ]; ret = RErr; - permitted_states = [ Connected ]; + permitted_states = [ Negotiating; Connected ]; modifies_fd = true; shortdesc = "disconnect from the NBD server"; longdesc = "\ @@ -2678,7 +2680,7 @@ "shutdown", { 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)>. +needed, see L<nbd_aio_opt_abort(3)> and L<nbd_aio_disconnect(3)>. The C<flags> argument is a bitmask, including zero or more of the following shutdown flags: @@ -2693,12 +2695,21 @@ "shutdown", { issuing those commands before informing the server of the intent to disconnect. +=item C<LIBNBD_SHUTDOWN_COVER_OPT_MODE> = 0x20000 + +If the server is still in option negotiation mode +(see L<nbd_set_opt_mode(3)>), gracefully abandon the +connection as if by L<nbd_opt_abort(3)>, instead of +complaining that the server is not yet fully connected. +If option negotiation mode was not in use or was +completed by L<nbd_opt_go(3)>, this flag has no effect. + =back For convenience, the constant C<LIBNBD_SHUTDOWN_MASK> is available to describe all shutdown flags recognized by this build of libnbd. A future version of the library may add new flags."; - see_also = [Link "close"; Link "aio_disconnect"]; + see_also = [Link "close"; Link "aio_disconnect"; Link "aio_opt_abort"]; example = Some "examples/reads-and-writes.c"; }; diff --git a/lib/disconnect.c b/lib/disconnect.c index 083d6cd3..d250e739 100644 --- a/lib/disconnect.c +++ b/lib/disconnect.c @@ -30,6 +30,20 @@ int nbd_unlocked_shutdown (struct nbd_handle *h, uint32_t flags) { + /* If still negotiating, return an error unless COVER_OPT_ABORT lets + * us trigger opt_abort instead. + */ + if (nbd_internal_is_state_negotiating (get_next_state (h))) { + if (flags & LIBNBD_SHUTDOWN_COVER_OPT_MODE) { + if (nbd_unlocked_aio_opt_abort (h) == -1) + return -1; + goto wait; + } + set_error (EINVAL, "invalid state: COVER_OPT_ABORT not requested, but " + "the handle is still negotiating options with the server"); + return -1; + } + /* If ABANDON_PENDING, abort any commands that have not yet had any * bytes sent to the server, so NBD_CMD_DISC becomes next in line. */ @@ -50,6 +64,7 @@ nbd_unlocked_shutdown (struct nbd_handle *h, uint32_t flags) return -1; } + wait: while (!nbd_internal_is_state_closed (get_next_state (h)) && !nbd_internal_is_state_dead (get_next_state (h))) { if (nbd_unlocked_poll (h, -1) == -1) diff --git a/tests/shutdown-opt-mode.c b/tests/shutdown-opt-mode.c index 34386220..96547607 100644 --- a/tests/shutdown-opt-mode.c +++ b/tests/shutdown-opt-mode.c @@ -119,6 +119,31 @@ main (int argc, char *argv[]) } nbd_close (nbd); - /* Part 3: TODO: add flag to nbd_shutdown */ + /* Part 3: Use of flag avoids the need to manually do opt_abort. */ + nbd = nbd_create (); + if (nbd == NULL) { + fprintf (stderr, "%s: %s\n", progname, nbd_get_error ()); + exit (EXIT_FAILURE); + } + if (nbd_set_opt_mode (nbd, true) == -1) { + fprintf (stderr, "%s: %s\n", progname, nbd_get_error ()); + exit (EXIT_FAILURE); + } + if (nbd_connect_command (nbd, (char **)cmd_new) == -1) { + fprintf (stderr, "%s: %s\n", progname, nbd_get_error ()); + exit (EXIT_FAILURE); + } + if (nbd_aio_is_negotiating (nbd) != 1) { + fprintf (stderr, "%s: unexpected state\n", progname); + exit (EXIT_FAILURE); + } + + /* Shutdown succeeds, because of the flag. */ + if (nbd_shutdown (nbd, LIBNBD_SHUTDOWN_COVER_OPT_MODE) == -1) { + fprintf (stderr, "%s: %s\n", progname, nbd_get_error ()); + exit (EXIT_FAILURE); + } + nbd_close (nbd); + exit (EXIT_SUCCESS); } -- 2.41.0

On Tue, Sep 05, 2023 at 08:47:34AM -0500, Eric Blake wrote: Actually I think a problem with this is that it always causes a segfault, which could leave a core dump around (does on my machine). It's unfortunate, because obviously you can't test for a double-free in nbdkit without causing a double-free. It might be a rare case when a version test is better. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org

On Thu, Sep 07, 2023 at 11:35:27AM -0500, Eric Blake wrote: I'll do a release. Thanks, Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top