Eric Blake
2020-Aug-11 02:09 UTC
[Libguestfs] [libnbd PATCH] API: Add nbd_set_opt_mode to expose NEGOTIATING state
This is the bare minimum needed to allow the user to take control over
the rest of option negotiating. This patch adds several new API:
nbd_set_opt_mode() - called during Created to enable the new mode
nbd_get_opt_mode() - query whether opt mode is enabled
nbd_opt_go() - used in Negotiating state to attempt to use export
nbd_opt_abort() - used in Negotiating state to skip Connected state
nbd_aio_is_negotiating() - used to detect Negotiating state
Older clients that do not use nbd_set_opt_mode see no difference: the
new state is never reached, all configuration (export name, requesting
meta contexts) has to be done in Created state, and the state machine
moves to READY on success. But newer clients that opt in gain a new
state Negotiating, reached after TLS and structured replies have been
negotiated, and prior to attempting NBD_OPT_GO. In fact, this state
can be reached multiple times, as a failed NBD_OPT_GO can now be
recovered from (such as attempting to set a different export name).
The list-exports example is updated to use the new API, in order to
demonstrate that it is now possible to do everything with just one nbd
handle.
The idea is that future patches will rework the NBD_OPT_LIST code to
instead be under the control of the user that enables opt mode,
probably with a callback function rather than malloc'ing the list in
the handle. In fact, by making TLS negotiating (when tls=1) optional,
libnbd can be used to test 'nbdkit --tls=on' as both a plaintext and
encrypted client, under user control. Similarly, NBD_OPT_INFO support
should be trivial to add (reuse most of the NBD_OPT_GO state machine).
We may also want aio counterparts rather than always synchronously
waiting for the state machine to get back to negotiating or connected
states.
---
lib/internal.h | 4 +
generator/API.ml | 93 +++++++++++++++++--
generator/API.mli | 1 +
generator/C.ml | 4 +-
generator/state_machine.ml | 26 ++++++
generator/states-magic.c | 6 +-
generator/states-newstyle-opt-go.c | 4 +
.../states-newstyle-opt-structured-reply.c | 7 +-
generator/states-newstyle.c | 47 ++++++++--
lib/connect.c | 5 +-
lib/is-state.c | 14 ++-
lib/Makefile.am | 3 +-
examples/list-exports.c | 64 +++++++++----
13 files changed, 235 insertions(+), 43 deletions(-)
diff --git a/lib/internal.h b/lib/internal.h
index 4d0c4e1..8e1fe79 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -98,6 +98,9 @@ struct nbd_handle {
int uri_allow_tls;
bool uri_allow_local_file;
+ /* Option negotiation mode. */
+ bool opt_mode;
+
/* List exports mode. */
bool list_exports;
size_t nr_exports;
@@ -398,6 +401,7 @@ extern int nbd_internal_set_block_size (struct nbd_handle
*h, uint32_t min,
/* is-state.c */
extern bool nbd_internal_is_state_created (enum state state);
extern bool nbd_internal_is_state_connecting (enum state state);
+extern bool nbd_internal_is_state_negotiating (enum state state);
extern bool nbd_internal_is_state_ready (enum state state);
extern bool nbd_internal_is_state_processing (enum state state);
extern bool nbd_internal_is_state_dead (enum state state);
diff --git a/generator/API.ml b/generator/API.ml
index 82fdf75..ff7f4c1 100644
--- a/generator/API.ml
+++ b/generator/API.ml
@@ -90,6 +90,7 @@ and flags = {
and permitted_state | Created
| Connecting
+| Negotiating
| Connected
| Closed | Dead
and link @@ -268,7 +269,7 @@ C<\"nbd2\">, etc.";
"set_export_name", {
default_call with
args = [ String "export_name" ]; ret = RErr;
- permitted_states = [ Created ];
+ permitted_states = [ Created; Negotiating ];
shortdesc = "set the export name";
longdesc = "\
For servers which require an export name or can serve different
@@ -361,7 +362,7 @@ on a particular connection use
L<nbd_get_tls_negotiated(3)> instead.";
"get_tls_negotiated", {
default_call with
args = []; ret = RBool;
- permitted_states = [ Connected; Closed ];
+ permitted_states = [ Negotiating; Connected; Closed ];
shortdesc = "find out if TLS was negotiated on a connection";
longdesc = "\
After connecting you may call this to find out if the
@@ -525,7 +526,7 @@ C<nbd_get_structured_replies_negotiated>
instead.";
"get_structured_replies_negotiated", {
default_call with
args = []; ret = RBool;
- permitted_states = [ Connected; Closed ];
+ permitted_states = [ Negotiating; Connected; Closed ];
shortdesc = "see if structured replies are in use";
longdesc = "\
After connecting you may call this to find out if the connection is
@@ -596,6 +597,65 @@ the time of compilation.";
Link "aio_is_created"; Link
"aio_is_ready"];
};
+ "set_opt_mode", {
+ default_call with
+ args = [Bool "enable"]; ret = RErr;
+ permitted_states = [ Created ];
+ shortdesc = "set whether to allow user control over option
negotiation";
+ longdesc = "\
+Set this flag to true in order to enter option negotiating mode with a
+newstyle server.
+
+In this mode, you have fine-grained control over which options are
+negotiated, compared to the default of the server negotiating everything
+on your behalf using settings made before starting the connection. To
+leave the mode and proceed on to the ready state, you must use
+C<nbd_opt_go> successfully. You may also use C<nbd_opt_abort> to
end
+the connection without finishing negotiation.";
+ example = Some "examples/list-exports.c";
+ see_also = [Link "get_opt_mode"; Link "opt_go"; Link
"opt_abort";
+ Link "get_list_exports";
+ Link "get_nr_list_exports"; Link
"get_list_export_name"];
+ };
+
+ "get_opt_mode", {
+ default_call with
+ args = []; ret = RBool;
+ may_set_error = false;
+ shortdesc = "return whether option mode was enabled";
+ longdesc = "\
+Return true if option negotiation mode was enabled on this handle.";
+ see_also = [Link "set_opt_mode"];
+ };
+
+ "opt_go", {
+ default_call with
+ args = []; ret = RErr;
+ permitted_states = [ Negotiating ];
+ shortdesc = "end negotiation and move on to using an export";
+ longdesc = "\
+Request that the server finish negotiation and move on to serving the
+export previously specified by C<nbd_set_export_name>.
+
+If this fails, the server may still be in negotiation, where it is
+possible to attempt another option such as a different export name;
+although older servers will instead have killed the connection.";
+ example = Some "examples/list-exports.c";
+ see_also = [Link "set_opt_mode"; Link "opt_abort"];
+ };
+
+ "opt_abort", {
+ default_call with
+ args = []; ret = RErr;
+ permitted_states = [ Negotiating ];
+ shortdesc = "end negotiation and close the connection";
+ longdesc = "\
+Request that the server finish negotiation, gracefully if possible, then
+close the connection.";
+ example = Some "examples/list-exports.c";
+ see_also = [Link "set_opt_mode"; Link "opt_go"];
+ };
+
"set_list_exports", {
default_call with
args = [Bool "list"]; ret = RErr;
@@ -649,7 +709,7 @@ Return true if list exports mode was enabled on this
handle.";
"get_nr_list_exports", {
default_call with
args = []; ret = RInt;
- permitted_states = [ Connected; Closed; Dead ];
+ permitted_states = [ Negotiating; Connected; Closed; Dead ];
shortdesc = "return the number of exports returned by the
server";
longdesc = "\
If list exports mode was enabled on the handle and you connected
@@ -663,7 +723,7 @@ C<nbd_set_list_exports>.";
"get_list_export_name", {
default_call with
args = [ Int "i" ]; ret = RString;
- permitted_states = [ Connected; Closed; Dead ];
+ permitted_states = [ Negotiating; Connected; Closed; Dead ];
shortdesc = "return the i'th export name";
longdesc = "\
If list exports mode was enabled on the handle and you connected
@@ -675,7 +735,7 @@ from the list returned by the server.";
"get_list_export_description", {
default_call with
args = [ Int "i" ]; ret = RString;
- permitted_states = [ Connected; Closed; Dead ];
+ permitted_states = [ Negotiating; Connected; Closed; Dead ];
shortdesc = "return the i'th export description";
longdesc = "\
If list exports mode was enabled on the handle and you connected
@@ -690,7 +750,7 @@ is set with I<-D>.";
"add_meta_context", {
default_call with
args = [ String "name" ]; ret = RErr;
- permitted_states = [ Created ];
+ permitted_states = [ Created; Negotiating ];
shortdesc = "ask server to negotiate metadata context";
longdesc = "\
During connection libnbd can negotiate zero or more metadata
@@ -1240,7 +1300,7 @@ are free to pass in other contexts."
"get_protocol", {
default_call with
args = []; ret = RStaticString;
- permitted_states = [ Connected; Closed ];
+ permitted_states = [ Negotiating; Connected; Closed ];
shortdesc = "return the NBD protocol variant";
longdesc = "\
Return the NBD protocol variant in use on the connection. At
@@ -2051,6 +2111,18 @@ issue commands (see L<nbd_aio_is_ready(3)>).";
see_also = [Link "aio_is_ready"];
};
+ "aio_is_negotiating", {
+ default_call with
+ args = []; ret = RBool; is_locked = false; may_set_error = false;
+ shortdesc = "check if the connection is handshaking";
+ longdesc = "\
+Return true if this connection is ready to start another option
+negotiation command while handshaking with the server, before
+the handle becomes ready to issue commands (see L<nbd_aio_is_ready(3)>).
+This state can only be reached if L<nbd_set_opt_mode> was used.";
+ see_also = [Link "aio_is_ready"; Link "set_opt_mode"];
+ };
+
"aio_is_ready", {
default_call with
args = []; ret = RBool; is_locked = false; may_set_error = false;
@@ -2355,6 +2427,11 @@ let first_version = [
"get_list_export_name", (1, 4);
"get_list_export_description", (1, 4);
"get_block_size", (1, 4);
+ "set_opt_mode", (1, 4);
+ "get_opt_mode", (1, 4);
+ "opt_go", (1, 4);
+ "opt_abort", (1, 4);
+ "aio_is_negotiating", (1, 4);
(* These calls are proposed for a future version of libnbd, but
* have not been added to any released version so far.
diff --git a/generator/API.mli b/generator/API.mli
index 3dbd1ff..712e837 100644
--- a/generator/API.mli
+++ b/generator/API.mli
@@ -102,6 +102,7 @@ and flags = {
and permitted_state | Created (** can be called in the START
state *)
| Connecting (** can be called when connecting/handshaking *)
+| Negotiating (** can be called when handshaking in opt_mode *)
| Connected (** when connected and READY or processing, but
not including CLOSED or DEAD *)
| Closed | Dead (** can be called when the handle is
diff --git a/generator/C.ml b/generator/C.ml
index afb8142..6b65f6e 100644
--- a/generator/C.ml
+++ b/generator/C.ml
@@ -397,7 +397,8 @@ let permitted_state_text permitted_states function
| Created -> "newly created"
| Connecting -> "connecting"
- | Connected -> "connected and finished handshaking with the
server"
+ | Negotiating -> "negotiating"
+ | Connected -> "connected with the server"
| Closed -> "shut down"
| Dead -> "dead"
) permitted_states
@@ -421,6 +422,7 @@ let generate_lib_api_c () function
| Created -> "nbd_internal_is_state_created (state)"
| Connecting -> "nbd_internal_is_state_connecting
(state)"
+ | Negotiating -> "nbd_internal_is_state_negotiating
(state)"
| Connected -> "nbd_internal_is_state_ready (state) ||
nbd_internal_is_state_processing (state)"
| Closed -> "nbd_internal_is_state_closed (state)"
| Dead -> "nbd_internal_is_state_dead (state)"
diff --git a/generator/state_machine.ml b/generator/state_machine.ml
index 144a5f8..617cbf3 100644
--- a/generator/state_machine.ml
+++ b/generator/state_machine.ml
@@ -100,6 +100,13 @@ let rec state_machine = [
Group ("OLDSTYLE", oldstyle_state_machine);
Group ("NEWSTYLE", newstyle_state_machine);
+ State {
+ default_state with
+ name = "NEGOTIATING";
+ comment = "Connection is ready to negotiate an NBD option";
+ external_events = [ CmdIssue, "NEWSTYLE.START" ];
+ };
+
State {
default_state with
name = "READY";
@@ -271,6 +278,8 @@ and newstyle_state_machine = [
(* Options. These state groups are always entered unconditionally,
* in this order. The START state in each group will check if the
* state needs to run and skip to the next state in the list if not.
+ * When opt_mode is set, control is returned to the user in state
+ * NEGOTIATING after OPT_STRUCTURED_REPLY or any failed OPT_GO.
*)
Group ("OPT_STARTTLS", newstyle_opt_starttls_state_machine);
Group ("OPT_LIST", newstyle_opt_list_state_machine);
@@ -279,6 +288,23 @@ and newstyle_state_machine = [
Group ("OPT_GO", newstyle_opt_go_state_machine);
Group ("OPT_EXPORT_NAME", newstyle_opt_export_name_state_machine);
+ (* When opt_mode is enabled, option parsing can be cleanly ended without
+ * moving through the %READY state.
+ *)
+ State {
+ default_state with
+ name = "SEND_OPT_ABORT";
+ comment = "Send NBD_OPT_ABORT to end negotiation";
+ external_events = [ NotifyWrite, "" ];
+ };
+
+ State {
+ default_state with
+ name = "SEND_OPTION_SHUTDOWN";
+ comment = "Sending write shutdown notification to the remote
server";
+ external_events = [ NotifyWrite, "" ];
+ };
+
(* When option parsing has successfully finished negotiation
* it will jump to this state for final steps before moving to
* the %READY state.
diff --git a/generator/states-magic.c b/generator/states-magic.c
index 944728d..2ad3a96 100644
--- a/generator/states-magic.c
+++ b/generator/states-magic.c
@@ -1,5 +1,5 @@
/* nbd client library in userspace: state machine
- * Copyright (C) 2013-2019 Red Hat Inc.
+ * 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
@@ -42,8 +42,10 @@ STATE_MACHINE {
}
version = be64toh (h->sbuf.new_handshake.version);
- if (version == NBD_NEW_VERSION)
+ if (version == NBD_NEW_VERSION) {
+ h->sbuf.option.option = 0;
SET_NEXT_STATE (%.NEWSTYLE.START);
+ }
else if (version == NBD_OLD_VERSION)
SET_NEXT_STATE (%.OLDSTYLE.START);
else {
diff --git a/generator/states-newstyle-opt-go.c
b/generator/states-newstyle-opt-go.c
index 1e75e0a..daafe91 100644
--- a/generator/states-newstyle-opt-go.c
+++ b/generator/states-newstyle-opt-go.c
@@ -202,6 +202,10 @@ STATE_MACHINE {
set_error (0, "handshake: unknown reply from NBD_OPT_GO: 0x%"
PRIx32,
reply);
}
+ if (h->opt_mode) {
+ SET_NEXT_STATE (%.NEGOTIATING);
+ return 0;
+ }
}
SET_NEXT_STATE (%.DEAD);
return 0;
diff --git a/generator/states-newstyle-opt-structured-reply.c
b/generator/states-newstyle-opt-structured-reply.c
index cfe6e0d..d0ac4c5 100644
--- a/generator/states-newstyle-opt-structured-reply.c
+++ b/generator/states-newstyle-opt-structured-reply.c
@@ -1,5 +1,5 @@
/* nbd client library in userspace: state machine
- * Copyright (C) 2013-2019 Red Hat Inc.
+ * 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
@@ -83,7 +83,10 @@ STATE_MACHINE {
}
/* Next option. */
- SET_NEXT_STATE (%^OPT_SET_META_CONTEXT.START);
+ if (h->opt_mode)
+ SET_NEXT_STATE (%.NEGOTIATING);
+ else
+ SET_NEXT_STATE (%^OPT_SET_META_CONTEXT.START);
return 0;
} /* END STATE MACHINE */
diff --git a/generator/states-newstyle.c b/generator/states-newstyle.c
index 573f724..e61f373 100644
--- a/generator/states-newstyle.c
+++ b/generator/states-newstyle.c
@@ -1,5 +1,5 @@
/* nbd client library in userspace: state machine
- * Copyright (C) 2013-2019 Red Hat Inc.
+ * 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
@@ -112,6 +112,25 @@ handle_reply_error (struct nbd_handle *h)
STATE_MACHINE {
NEWSTYLE.START:
+ if (h->opt_mode) {
+ switch (h->sbuf.option.option) {
+ case NBD_OPT_GO:
+ SET_NEXT_STATE (%OPT_SET_META_CONTEXT.START);
+ return 0;
+ case NBD_OPT_ABORT:
+ h->sbuf.option.version = htobe64 (NBD_NEW_VERSION);
+ h->sbuf.option.option = htobe32 (NBD_OPT_ABORT);
+ h->sbuf.option.optlen = htobe32 (0);
+ h->wbuf = &h->sbuf;
+ h->wlen = sizeof h->sbuf.option;
+ SET_NEXT_STATE (%SEND_OPT_ABORT);
+ return 0;
+ case 0:
+ break;
+ default:
+ abort ();
+ }
+ }
h->rbuf = &h->sbuf;
h->rlen = sizeof h->sbuf.gflags;
SET_NEXT_STATE (%RECV_GFLAGS);
@@ -136,6 +155,11 @@ STATE_MACHINE {
return 0;
}
+ if ((h->gflags & LIBNBD_HANDSHAKE_FLAG_FIXED_NEWSTYLE) == 0)
+ h->protocol = "newstyle";
+ else
+ h->protocol = "newstyle-fixed";
+
cflags = h->gflags;
h->sbuf.cflags = htobe32 (cflags);
h->wbuf = &h->sbuf;
@@ -155,12 +179,23 @@ STATE_MACHINE {
}
return 0;
+ NEWSTYLE.SEND_OPT_ABORT:
+ assert ((h->gflags & LIBNBD_HANDSHAKE_FLAG_FIXED_NEWSTYLE) != 0);
+ assert (h->opt_mode);
+ switch (send_from_wbuf (h)) {
+ case -1: SET_NEXT_STATE (%.DEAD); return 0;
+ case 0:
+ SET_NEXT_STATE (%SEND_OPTION_SHUTDOWN);
+ }
+ return 0;
+
+ NEWSTYLE.SEND_OPTION_SHUTDOWN:
+ /* We don't care if the server replies to NBD_OPT_ABORT */
+ if (h->sock->ops->shut_writes (h, h->sock))
+ SET_NEXT_STATE (%.CLOSED);
+ return 0;
+
NEWSTYLE.FINISHED:
- if ((h->gflags & LIBNBD_HANDSHAKE_FLAG_FIXED_NEWSTYLE) == 0)
- h->protocol = "newstyle";
- else
- h->protocol = "newstyle-fixed";
-
SET_NEXT_STATE (%.READY);
return 0;
diff --git a/lib/connect.c b/lib/connect.c
index 92ed6b1..7e42b14 100644
--- a/lib/connect.c
+++ b/lib/connect.c
@@ -1,5 +1,5 @@
/* NBD client library in userspace
- * Copyright (C) 2013-2019 Red Hat Inc.
+ * 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
@@ -40,7 +40,8 @@
static int
error_unless_ready (struct nbd_handle *h)
{
- if (nbd_internal_is_state_ready (get_next_state (h)))
+ if (nbd_internal_is_state_ready (get_next_state (h)) ||
+ nbd_internal_is_state_negotiating (get_next_state (h)))
return 0;
/* Why did it fail? */
diff --git a/lib/is-state.c b/lib/is-state.c
index 1a85c7a..e019e53 100644
--- a/lib/is-state.c
+++ b/lib/is-state.c
@@ -1,5 +1,5 @@
/* NBD client library in userspace
- * Copyright (C) 2013-2019 Red Hat Inc.
+ * 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
@@ -58,6 +58,12 @@ nbd_internal_is_state_connecting (enum state state)
return is_connecting_group (group);
}
+bool
+nbd_internal_is_state_negotiating (enum state state)
+{
+ return state == STATE_NEGOTIATING;
+}
+
bool
nbd_internal_is_state_ready (enum state state)
{
@@ -120,6 +126,12 @@ nbd_unlocked_aio_is_connecting (struct nbd_handle *h)
return nbd_internal_is_state_connecting (get_public_state (h));
}
+int
+nbd_unlocked_aio_is_negotiating (struct nbd_handle *h)
+{
+ return nbd_internal_is_state_negotiating (get_public_state (h));
+}
+
int
nbd_unlocked_aio_is_ready (struct nbd_handle *h)
{
diff --git a/lib/Makefile.am b/lib/Makefile.am
index 1c46c54..9fd6331 100644
--- a/lib/Makefile.am
+++ b/lib/Makefile.am
@@ -1,5 +1,5 @@
# nbd client library in userspace
-# Copyright (C) 2013-2019 Red Hat Inc.
+# 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
@@ -47,6 +47,7 @@ libnbd_la_SOURCES = \
internal.h \
is-state.c \
nbd-protocol.h \
+ opt.c \
poll.c \
protocol.c \
rw.c \
diff --git a/examples/list-exports.c b/examples/list-exports.c
index 643e611..daea2ab 100644
--- a/examples/list-exports.c
+++ b/examples/list-exports.c
@@ -4,9 +4,27 @@
* $ qemu-nbd -x "hello" -t -k /tmp/sock disk.img
* $ ./run examples/list-exports /tmp/sock
* [0] hello
- * Which export to connect to? 0
+ * Which export to connect to (-1 to quit)? 0
* Connecting to hello ...
* /tmp/sock: hello: size = 2048 bytes
+ *
+ * To test this with nbdkit (requires 1.22):
+ * $ nbdkit -U /tmp/sock sh - <<\EOF
+ * case $1 in
+ * list_exports) echo NAMES; echo foo; echo foobar ;;
+ * open) echo "$3" ;;
+ * get_size) echo "$2" | wc -c ;;
+ * pread) echo "$2" | dd skip=$4 count=$3 \
+ * iflag=skip_bytes,count_bytes ;;
+ * *) exit 2 ;;
+ * esac
+ * EOF
+ * $ ./run examples/list-exports /tmp/sock
+ * [0] foo
+ * [1] foobar
+ * Which export to connect to (-1 to quit)? 1
+ * Connecting to foobar ...
+ * /tmp/sock: foobar: size = 7 bytes
*/
#include <stdio.h>
@@ -20,7 +38,7 @@
int
main (int argc, char *argv[])
{
- struct nbd_handle *nbd, *nbd2;
+ struct nbd_handle *nbd;
int r, i;
char *name, *desc;
int64_t size;
@@ -30,14 +48,15 @@ main (int argc, char *argv[])
exit (EXIT_FAILURE);
}
- /* Create the libnbd handle for querying exports. */
+ /* Create the libnbd handle. */
nbd = nbd_create ();
if (nbd == NULL) {
fprintf (stderr, "%s\n", nbd_get_error ());
exit (EXIT_FAILURE);
}
- /* Set the list exports mode in the handle. */
+ /* Set opt mode and request list exports. */
+ nbd_set_opt_mode (nbd, true);
nbd_set_list_exports (nbd, true);
/* Connect to the NBD server over a
@@ -52,8 +71,8 @@ main (int argc, char *argv[])
fprintf (stderr, "%s\n", nbd_get_error ());
exit (EXIT_FAILURE);
}
-
- if (nbd_get_nr_list_exports (nbd) == 0) {
+ if (!nbd_aio_is_negotiating (nbd) ||
+ nbd_get_nr_list_exports (nbd) == 0) {
fprintf (stderr, "Server does not support "
"listing exports.\n");
exit (EXIT_FAILURE);
@@ -73,29 +92,34 @@ main (int argc, char *argv[])
}
printf ("Which export to connect to? ");
if (scanf ("%d", &i) != 1) exit (EXIT_FAILURE);
+ if (i == -1) {
+ if (nbd_opt_abort (nbd) == -1) {
+ fprintf (stderr, "%s\n", nbd_get_error ());
+ exit (EXIT_FAILURE);
+ }
+ nbd_close (nbd);
+ exit (EXIT_SUCCESS);
+ }
name = nbd_get_list_export_name (nbd, i);
+ if (name == NULL) {
+ fprintf (stderr, "%s\n", nbd_get_error ());
+ exit (EXIT_FAILURE);
+ }
printf ("Connecting to %s ...\n", name);
- nbd_close (nbd);
- /* Connect again to the chosen export. */
- nbd2 = nbd_create ();
- if (nbd2 == NULL) {
+ /* Resume connecting to the chosen export. */
+ if (nbd_set_export_name (nbd, name) == -1 ||
+ nbd_opt_go (nbd) == -1) {
fprintf (stderr, "%s\n", nbd_get_error ());
exit (EXIT_FAILURE);
}
-
- if (nbd_set_export_name (nbd2, name) == -1) {
- fprintf (stderr, "%s\n", nbd_get_error ());
- exit (EXIT_FAILURE);
- }
-
- if (nbd_connect_unix (nbd2, argv[1]) == -1) {
- fprintf (stderr, "%s\n", nbd_get_error ());
+ if (!nbd_aio_is_ready (nbd)) {
+ fprintf (stderr, "server closed early\n");
exit (EXIT_FAILURE);
}
/* Read the size in bytes and print it. */
- size = nbd_get_size (nbd2);
+ size = nbd_get_size (nbd);
if (size == -1) {
fprintf (stderr, "%s\n", nbd_get_error ());
exit (EXIT_FAILURE);
@@ -104,7 +128,7 @@ main (int argc, char *argv[])
argv[1], name, size);
/* Close the libnbd handle. */
- nbd_close (nbd2);
+ nbd_close (nbd);
free (name);
--
2.28.0
Eric Blake
2020-Aug-11 02:13 UTC
Re: [Libguestfs] [libnbd PATCH] API: Add nbd_set_opt_mode to expose NEGOTIATING state
On 8/10/20 9:09 PM, Eric Blake wrote:> This is the bare minimum needed to allow the user to take control over > the rest of option negotiating. This patch adds several new API: >> --- > lib/internal.h | 4 + > generator/API.ml | 93 +++++++++++++++++-- > generator/API.mli | 1 + > generator/C.ml | 4 +- > generator/state_machine.ml | 26 ++++++ > generator/states-magic.c | 6 +- > generator/states-newstyle-opt-go.c | 4 + > .../states-newstyle-opt-structured-reply.c | 7 +- > generator/states-newstyle.c | 47 ++++++++-- > lib/connect.c | 5 +- > lib/is-state.c | 14 ++- > lib/Makefile.am | 3 +- > examples/list-exports.c | 64 +++++++++---- > 13 files changed, 235 insertions(+), 43 deletions(-)It helps if I add my new file before making the commit: diff --git c/lib/opt.c i/lib/opt.c new file mode 100644 index 0000000..5d74a4f --- /dev/null +++ i/lib/opt.c @@ -0,0 +1,85 @@ +/* NBD client library in userspace + * Copyright (C) 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 + */ + +#include <config.h> + +#include <stdio.h> +#include <stdlib.h> +#include <stdbool.h> +#include <stdint.h> +#include <inttypes.h> +#include <errno.h> +#include <assert.h> +#include <limits.h> + +#include "internal.h" + +int +nbd_unlocked_set_opt_mode (struct nbd_handle *h, bool value) +{ + h->opt_mode = value; + return 0; +} + +/* NB: may_set_error = false. */ +int +nbd_unlocked_get_opt_mode (struct nbd_handle *h) +{ + return h->opt_mode; +} + +static int +wait_for_option (struct nbd_handle *h) +{ + while (nbd_internal_is_state_connecting (get_next_state (h))) { + if (nbd_unlocked_poll (h, -1) == -1) + return -1; + } + + /* Should we do further decoding, like connect.c error_unless_ready? */ + return 0; +} + +/* Issue NBD_OPT_GO (or NBD_OPT_EXPORT_NAME) and wait for the reply. */ +int +nbd_unlocked_opt_go (struct nbd_handle *h) +{ + int r; + + h->sbuf.option.option = NBD_OPT_GO; + + if (nbd_internal_run (h, cmd_issue) == -1) + debug (h, "option queued, ignoring state machine failure"); + + r = wait_for_option (h); + if (r == 0 && nbd_internal_is_state_negotiating (get_next_state (h))) + return -1; /* NBD_OPT_GO failed, but can be attempted again */ + return r; +} + +/* Issue NBD_OPT_ABORT and wait for the state change. */ +int +nbd_unlocked_opt_abort (struct nbd_handle *h) +{ + h->sbuf.option.option = NBD_OPT_ABORT; + + if (nbd_internal_run (h, cmd_issue) == -1) + debug (h, "option queued, ignoring state machine failure"); + + return wait_for_option (h); +} -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Richard W.M. Jones
2020-Aug-11 17:12 UTC
Re: [Libguestfs] [libnbd PATCH] API: Add nbd_set_opt_mode to expose NEGOTIATING state
I think this needs extra documentation in docs/libnbd.pod because it's definitely not clear just from the rather thin manual page for set_opt_mode how it works. docs/libnbd.pod is a good place for a broader description of how it works. IIUC let me try to explain: we get through as far as the end of group OPT_STRUCTURED_REPLY before we check the opt flag, and then the remainder of opt negotiation can be controlled by the caller. (I really need to write a states -> dot graph visualizer!) When we've got to the negotiating state we can then get the list of exports, set the export name we want, and then finish the connection with nbd_opt_go. Don't we need nbd_aio_opt_go etc? Is there a case where you might want to influence TLS negotiation? I can't think of one right now. Something about supplying a client password from the command line maybe. Should setting the opt flag cause a failure if we connect to an oldstyle server? (I can see arguments both ways.) The updated list-exports example certainly shows the advantage of the new API. More comments inline below ... On Mon, Aug 10, 2020 at 09:09:11PM -0500, Eric Blake wrote:> diff --git a/generator/states-magic.c b/generator/states-magic.c > index 944728d..2ad3a96 100644 > --- a/generator/states-magic.c > +++ b/generator/states-magic.c...> @@ -42,8 +42,10 @@ STATE_MACHINE { > } > > version = be64toh (h->sbuf.new_handshake.version); > - if (version == NBD_NEW_VERSION) > + if (version == NBD_NEW_VERSION) { > + h->sbuf.option.option = 0; > SET_NEXT_STATE (%.NEWSTYLE.START); > + } > else if (version == NBD_OLD_VERSION) > SET_NEXT_STATE (%.OLDSTYLE.START); > else {What does this hunk do?> diff --git a/generator/states-newstyle.c b/generator/states-newstyle.c > index 573f724..e61f373 100644 > --- a/generator/states-newstyle.c > +++ b/generator/states-newstyle.c > @@ -1,5 +1,5 @@ > /* nbd client library in userspace: state machine > - * Copyright (C) 2013-2019 Red Hat Inc. > + * 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 > @@ -112,6 +112,25 @@ handle_reply_error (struct nbd_handle *h) > > STATE_MACHINE { > NEWSTYLE.START: > + if (h->opt_mode) { > + switch (h->sbuf.option.option) { > + case NBD_OPT_GO: > + SET_NEXT_STATE (%OPT_SET_META_CONTEXT.START); > + return 0; > + case NBD_OPT_ABORT: > + h->sbuf.option.version = htobe64 (NBD_NEW_VERSION); > + h->sbuf.option.option = htobe32 (NBD_OPT_ABORT); > + h->sbuf.option.optlen = htobe32 (0); > + h->wbuf = &h->sbuf; > + h->wlen = sizeof h->sbuf.option; > + SET_NEXT_STATE (%SEND_OPT_ABORT); > + return 0; > + case 0: > + break; > + default: > + abort (); > + } > + } > h->rbuf = &h->sbuf; > h->rlen = sizeof h->sbuf.gflags; > SET_NEXT_STATE (%RECV_GFLAGS);And this hunk which I guess is related to above but I couldn't quite work out why it's needed either.> @@ -136,6 +155,11 @@ STATE_MACHINE { > return 0; > } > > + if ((h->gflags & LIBNBD_HANDSHAKE_FLAG_FIXED_NEWSTYLE) == 0) > + h->protocol = "newstyle"; > + else > + h->protocol = "newstyle-fixed"; > + > cflags = h->gflags; > h->sbuf.cflags = htobe32 (cflags); > h->wbuf = &h->sbuf;This makes me think maybe we should just derive this string from the gflags when the caller calls get_protocol.> diff --git a/lib/is-state.c b/lib/is-state.c > index 1a85c7a..e019e53 100644 > --- a/lib/is-state.c > +++ b/lib/is-state.c > @@ -1,5 +1,5 @@ > /* NBD client library in userspace > - * Copyright (C) 2013-2019 Red Hat Inc. > + * 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 > @@ -58,6 +58,12 @@ nbd_internal_is_state_connecting (enum state state) > return is_connecting_group (group); > } > > +bool > +nbd_internal_is_state_negotiating (enum state state) > +{ > + return state == STATE_NEGOTIATING; > +} > + > bool > nbd_internal_is_state_ready (enum state state) > {When I was reading the rest of the patch I thought it would have been easier to review if the addition of the new state was in a separate commit. 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
Eric Blake
2020-Aug-11 17:38 UTC
Re: [Libguestfs] [libnbd PATCH] API: Add nbd_set_opt_mode to expose NEGOTIATING state
On 8/11/20 12:12 PM, Richard W.M. Jones wrote:> I think this needs extra documentation in docs/libnbd.pod because it's > definitely not clear just from the rather thin manual page for > set_opt_mode how it works. docs/libnbd.pod is a good place for a > broader description of how it works.Yes, good idea. State-wise, the existing flow was: Created - Progress only by command issue - namely one of nbd_connect_* Connecting - Progress by aio_notify_read/aio_notify_write (as driven by aio_poll), and progresses through socket establishment, magic numbers, and handshaking Loop of: Ready - Progress by command issue (I/O commands) or aio_notify_read Processing - Progress by aio_notify_read/aio_notify_write, commands are queued rather than causing state transitions finally, end in Closed (clean disconnect) or Dead (dead socket or protocol error) the new flow breaks Connecting into two halves, adding one more point where command issue can cause a state transition: Created - Progress only by command issue - namely one of nbd_connect_* Connecting - Progress by aio_notify_read/aio_notify_write (as driven by aio_poll), and progresses through socket establishment and magic numbers Loop of: Negotiating - Progress by command issue (nbd_opt_*) Connecting - Progress by aio_notify_read/aio_notify_write, no commands accepted, and processes remaining handshaking Before getting back to Ready and the normal loop. One thing to remember is that handshaking is always synchronous - the client is not allowed to send the next NBD_OPT_* until after the server has finished replying to the previous. Since everything is in lockstep, nothing needs to be queued, and we don't have quite the complexity of Ready handling simultaneous command issue, read notify (for completion of previous commands) and write notify (when the outgoing queue can make progress again). The state machine still shows as Connecting as it makes progress towards Ready for anything that is not waiting on a user command, and skipping state Negotiating is all the more that is required for old clients to not see a difference. But where old clients jumped straight to Dead on any error reply to an NBD_OPT request, the new mode can jump back to Negotiating to give the client a chance to try something else, moving back into Connecting once a command is started.> > IIUC let me try to explain: we get through as far as the end of group > OPT_STRUCTURED_REPLY before we check the opt flag, and then the > remainder of opt negotiation can be controlled by the caller. (I > really need to write a states -> dot graph visualizer!) When we've > got to the negotiating state we can then get the list of exports, set > the export name we want, and then finish the connection with > nbd_opt_go.For now, OPT_STRUCTURED_REPLY is still automatic, although I might expose it to the user to attempt (I'm certainly thinking about what else to expose or keep automatic in followup patches; letting the user control OPT_STARTTLS when tls=1 (but not when tls=0 or tls=2) may be useful).> > Don't we need nbd_aio_opt_go etc?Probably - that was one of my questions. In fact, adding aio_opt_* is easy (the sync version would call the aio version, which kicks off the NBD_OPT_ write; the difference is that the aio version then returns immediately, probably in aio_is_connecting, while the sync version uses aio_poll until it is back to either aio_is_negotiating, aio_is_ready, or aio_is_dead).> > Is there a case where you might want to influence TLS negotiation? I > can't think of one right now. Something about supplying a client > password from the command line maybe. > > Should setting the opt flag cause a failure if we connect to an > oldstyle server? (I can see arguments both ways.) > > The updated list-exports example certainly shows the advantage of the > new API. > > More comments inline below ... > > On Mon, Aug 10, 2020 at 09:09:11PM -0500, Eric Blake wrote: >> diff --git a/generator/states-magic.c b/generator/states-magic.c >> index 944728d..2ad3a96 100644 >> --- a/generator/states-magic.c >> +++ b/generator/states-magic.c > ... >> @@ -42,8 +42,10 @@ STATE_MACHINE { >> } >> >> version = be64toh (h->sbuf.new_handshake.version); >> - if (version == NBD_NEW_VERSION) >> + if (version == NBD_NEW_VERSION) { >> + h->sbuf.option.option = 0; >> SET_NEXT_STATE (%.NEWSTYLE.START); >> + } >> else if (version == NBD_OLD_VERSION) >> SET_NEXT_STATE (%.OLDSTYLE.START); >> else { > > What does this hunk do? > >> diff --git a/generator/states-newstyle.c b/generator/states-newstyle.c >> index 573f724..e61f373 100644 >> --- a/generator/states-newstyle.c >> +++ b/generator/states-newstyle.c >> @@ -1,5 +1,5 @@ >> /* nbd client library in userspace: state machine >> - * Copyright (C) 2013-2019 Red Hat Inc. >> + * 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 >> @@ -112,6 +112,25 @@ handle_reply_error (struct nbd_handle *h) >> >> STATE_MACHINE { >> NEWSTYLE.START: >> + if (h->opt_mode) { >> + switch (h->sbuf.option.option) { >> + case NBD_OPT_GO: >> + SET_NEXT_STATE (%OPT_SET_META_CONTEXT.START); >> + return 0; >> + case NBD_OPT_ABORT: >> + h->sbuf.option.version = htobe64 (NBD_NEW_VERSION); >> + h->sbuf.option.option = htobe32 (NBD_OPT_ABORT); >> + h->sbuf.option.optlen = htobe32 (0); >> + h->wbuf = &h->sbuf; >> + h->wlen = sizeof h->sbuf.option; >> + SET_NEXT_STATE (%SEND_OPT_ABORT); >> + return 0; >> + case 0: >> + break; >> + default: >> + abort (); >> + } >> + } >> h->rbuf = &h->sbuf; >> h->rlen = sizeof h->sbuf.gflags; >> SET_NEXT_STATE (%RECV_GFLAGS); > > And this hunk which I guess is related to above but I couldn't quite > work out why it's needed either. > >> @@ -136,6 +155,11 @@ STATE_MACHINE { >> return 0; >> } >> >> + if ((h->gflags & LIBNBD_HANDSHAKE_FLAG_FIXED_NEWSTYLE) == 0) >> + h->protocol = "newstyle"; >> + else >> + h->protocol = "newstyle-fixed"; >> + >> cflags = h->gflags; >> h->sbuf.cflags = htobe32 (cflags); >> h->wbuf = &h->sbuf; > > This makes me think maybe we should just derive this string from the > gflags when the caller calls get_protocol.Doable. Probably even something we could separate out, to keep the meat of this patch more focused.>> +bool >> +nbd_internal_is_state_negotiating (enum state state) >> +{ >> + return state == STATE_NEGOTIATING; >> +} >> + >> bool >> nbd_internal_is_state_ready (enum state state) >> { > > When I was reading the rest of the patch I thought it would have been > easier to review if the addition of the new state was in a separate > commit.Sure; I'll try and make that split for v2, as well as adding aio_opt_*. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Possibly Parallel Threads
- [libnbd PATCH] API: Add nbd_set_opt_mode to expose NEGOTIATING state
- Re: [libnbd PATCH] API: Add nbd_set_opt_mode to expose NEGOTIATING state
- [libnbd PATCH v2 00/13] Adding nbd_set_opt_mode to improve nbdinfo
- Re: [libnbd PATCH] API: Add nbd_set_opt_mode to expose NEGOTIATING state
- [libnbd PATCH] api: Add set_handshake_flags for integration