5 p.m.
Adding NBD_OPT_LIST support will occur across two commits. This patch
switches the existing nbd_set_list_exports() (called at a distance
compared to where its results are used) to instead be the synchronous
nbd_opt_list() command; the next patch will then focus on making the
API friendlier to async usage by shifting from libnbd malloc'ing the
results to instead having the caller supply a callback function.
Thus, the API signature in this patch is not what we intend to be
final for 1.4, but rather a division to make review of the
transformation easier.
---
lib/internal.h | 3 +-
generator/API.ml | 84 ++++++++++--------------
generator/state_machine.ml | 4 +-
generator/states-newstyle-opt-list.c | 12 ++--
generator/states-newstyle-opt-starttls.c | 8 +--
generator/states-newstyle.c | 3 +
lib/handle.c | 26 ++------
lib/opt.c | 33 ++++++++++
tests/newstyle-limited.c | 9 ++-
examples/list-exports.c | 22 ++++---
interop/list-exports.c | 11 ++--
info/nbdinfo.c | 33 ++++------
12 files changed, 131 insertions(+), 117 deletions(-)
diff --git a/lib/internal.h b/lib/internal.h
index 3672538..3d059cc 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -103,8 +103,7 @@ struct nbd_handle {
bool opt_mode;
uint8_t current_opt;
- /* List exports mode. */
- bool list_exports;
+ /* Results of nbd_opt_list. */
size_t nr_exports;
struct export *exports;
diff --git a/generator/API.ml b/generator/API.ml
index 21485ea..c660960 100644
--- a/generator/API.ml
+++ b/generator/API.ml
@@ -288,12 +288,14 @@ The encoding of export names is always UTF-8.
When option mode is not in use, the export name must be set
before beginning a connection. However, when L<nbd_set_opt_mode(3)>
has enabled option mode, it is possible to change the export
-name prior to L<nbd_opt_go(3)>.
+name prior to L<nbd_opt_go(3)>. In particular, the use of
+L<nbd_opt_list(3)> during negotiation can be used to determine
+a name the server is likely to accept.
This call may be skipped if using L<nbd_connect_uri(3)> to connect
to a URI that includes an export name.";
see_also = [Link "get_export_name"; Link "connect_uri";
- Link "set_opt_mode"; Link "opt_go"];
+ Link "set_opt_mode"; Link "opt_go"; Link
"opt_list"];
};
"get_export_name", {
@@ -709,7 +711,7 @@ export name before trying again. You may also use
L<nbd_opt_abort(3)>
to end the connection without finishing negotiation.";
example = Some "examples/list-exports.c";
see_also = [Link "get_opt_mode"; Link "aio_is_negotiating";
- Link "opt_abort"; Link "opt_go"];
+ Link "opt_abort"; Link "opt_go"; Link
"opt_list"];
};
"get_opt_mode", {
@@ -753,68 +755,55 @@ enabled option mode.";
see_also = [Link "set_opt_mode"; Link "aio_opt_abort"; Link
"opt_go"];
};
- "set_list_exports", {
+ "opt_list", {
default_call with
- args = [Bool "list"]; ret = RErr;
- permitted_states = [ Created ];
- shortdesc = "set whether to list server exports";
+ args = []; ret = RErr;
+ permitted_states = [ Negotiating ];
+ shortdesc = "request the server to list all exports during negotiation";
longdesc = "\
-Set this flag to true in order to list NBD exports provided
-by the server.
+Request that the server list all exports that it supports. This can
+only be used if L<nbd_set_opt_mode(3)> enabled option mode.
In this mode, during connection we query the server for the list
of NBD exports and collect them in the handle. You can query
the list of exports provided by the server by calling
L<nbd_get_nr_list_exports(3)> and L<nbd_get_list_export_name(3)>.
-After choosing the export you want, you should close this handle,
-create a new NBD handle (L<nbd_create(3)>), set the export name
-(L<nbd_set_export_name(3)>), and connect on the new handle
-(C<nbd_connect_*>).
+After choosing the export you want, set the export name
+(L<nbd_set_export_name(3)>), then finish connecting with L<nbd_opt_go(3)>.
Some servers do not support listing exports at all. In
that case the connect call will fail with errno C<ENOTSUP>
and L<nbd_get_nr_list_exports(3)> will return 0.
-Some servers do not respond with all the exports they
-support, either because of an incomplete implementation of
-this feature, or because they only list exports relevant
-to non-TLS or TLS when a corresponding non-TLS or TLS
-connection is opened.
-
-For L<qemu-nbd(8)> when using the I<-x> option you may find
-that the original connect call fails with
-C<server has no export named ''> and errno C<ENOENT>.
-However you can still proceed to call L<nbd_get_nr_list_exports(3)> etc.
+Not all servers understand this request, and even when it is understood,
+the server might intentionally send an empty list to avoid being an
+information leak or may encounter a failure after delivering partial
+results. Thus, this function may succeed even when no exports
+are reported, or may fail but have a non-empty list. Likewise,
+the NBD protocol does not specify an upper bound for the number of
+exports that might be advertised, so client code should be aware that
+a server may send a lengthy list; libnbd truncates the server reply
+after 10000 exports.
For L<nbd-server(1)> you will need to allow clients to make
list requests by adding C<allowlist=true> to the C<[generic]>
-section of F</etc/nbd-server/config>.";
+section of F</etc/nbd-server/config>. For L<qemu-nbd(8)>, a
+description is set with I<-D>.";
example = Some "examples/list-exports.c";
- see_also = [Link "get_list_exports";
+ see_also = [Link "set_opt_mode"; Link "opt_go";
Link "get_nr_list_exports"; Link
"get_list_export_name"];
};
- "get_list_exports", {
- default_call with
- args = []; ret = RBool;
- may_set_error = false;
- shortdesc = "return whether list exports mode was enabled";
- longdesc = "\
-Return true if list exports mode was enabled on this handle.";
- see_also = [Link "set_list_exports"];
- };
-
"get_nr_list_exports", {
default_call with
args = []; ret = RInt;
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
-to the server, this returns the number of exports returned by the
-server. This may be 0 or incomplete for reasons given in
-L<nbd_set_list_exports(3)>.";
- see_also = [Link "set_list_exports"; Link
"get_list_export_name";
+If option mode is in effect and you called L<nbd_opt_list(3)>,
+this returns the number of exports returned by the server. This
+may be 0 or incomplete for reasons given in L<nbd_opt_list(3)>.";
+ see_also = [Link "opt_list"; Link "get_list_export_name";
Link "get_list_export_description"];
};
@@ -824,10 +813,10 @@ L<nbd_set_list_exports(3)>.";
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
-to the server, this can be used to return the i'th export name
+If L<nbd_opt_list(3)> succeeded with option mode enabled,
+this can be used to return the i'th export name
from the list returned by the server.";
- see_also = [Link "set_list_exports"; Link
"get_list_export_description"];
+ see_also = [Link "opt_list"; Link
"get_list_export_description"];
};
"get_list_export_description", {
@@ -836,13 +825,13 @@ from the list returned by the server.";
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
-to the server, this can be used to return the i'th export description
+If L<nbd_opt_list(3)> succeeded with option mode enabled,
+this can be used to return the i'th export description
from the list returned by the server, which may be an empty string.
Many servers omit a description. For L<qemu-nbd(8)>, a description
is set with I<-D>.";
- see_also = [Link "set_list_exports"; Link
"get_list_export_name"];
+ see_also = [Link "set_opt_mode"; Link "opt_go"];
};
"add_meta_context", {
@@ -2559,8 +2548,6 @@ let first_version = [
"set_uri_allow_local_file", (1, 2);
(* Added in 1.3.x development cycle, will be stable and supported in 1.4. *)
- "set_list_exports", (1, 4);
- "get_list_exports", (1, 4);
"get_nr_list_exports", (1, 4);
"get_list_export_name", (1, 4);
"get_list_export_description", (1, 4);
@@ -2574,6 +2561,7 @@ let first_version = [
"aio_is_negotiating", (1, 4);
"opt_go", (1, 4);
"opt_abort", (1, 4);
+ "opt_list", (1, 4);
"aio_opt_go", (1, 4);
"aio_opt_abort", (1, 4);
diff --git a/generator/state_machine.ml b/generator/state_machine.ml
index 2d28c2e..c1fb073 100644
--- a/generator/state_machine.ml
+++ b/generator/state_machine.ml
@@ -282,12 +282,14 @@ and newstyle_state_machine = [
* 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);
Group ("OPT_STRUCTURED_REPLY", newstyle_opt_structured_reply_state_machine);
Group ("OPT_SET_META_CONTEXT", newstyle_opt_set_meta_context_state_machine);
Group ("OPT_GO", newstyle_opt_go_state_machine);
Group ("OPT_EXPORT_NAME", newstyle_opt_export_name_state_machine);
+ (* Options that can be used during negotiation, when opt_mode is enabled. *)
+ Group ("OPT_LIST", newstyle_opt_list_state_machine);
+
(* When NBD_OPT_GO fails, or when opt_mode is enabled, option parsing
* can be cleanly ended without moving through the %READY state.
*)
diff --git a/generator/states-newstyle-opt-list.c b/generator/states-newstyle-opt-list.c
index f2846b6..19b353e 100644
--- a/generator/states-newstyle-opt-list.c
+++ b/generator/states-newstyle-opt-list.c
@@ -18,17 +18,13 @@
/* State machine for sending NBD_OPT_LIST to list exports.
*
- * This is skipped unless list exports mode was enabled on the handle.
+ * This is only reached via nbd_opt_list during opt_mode.
*/
STATE_MACHINE {
NEWSTYLE.OPT_LIST.START:
assert (h->gflags & LIBNBD_HANDSHAKE_FLAG_FIXED_NEWSTYLE);
- if (!h->list_exports) {
- SET_NEXT_STATE (%^OPT_STRUCTURED_REPLY.START);
- return 0;
- }
-
+ assert (h->opt_mode && h->exports && !h->nr_exports);
h->sbuf.option.version = htobe64 (NBD_NEW_VERSION);
h->sbuf.option.option = htobe32 (NBD_OPT_LIST);
h->sbuf.option.optlen = 0;
@@ -135,7 +131,7 @@ STATE_MACHINE {
case NBD_REP_ACK:
/* Finished receiving the list. */
- SET_NEXT_STATE (%^OPT_STRUCTURED_REPLY.START);
+ SET_NEXT_STATE (%.NEGOTIATING);
return 0;
default:
@@ -145,7 +141,7 @@ STATE_MACHINE {
}
set_error (ENOTSUP, "unexpected response, possibly the server does not "
"support listing exports");
- SET_NEXT_STATE (%.DEAD);
+ SET_NEXT_STATE (%.NEGOTIATING);
return 0;
}
return 0;
diff --git a/generator/states-newstyle-opt-starttls.c
b/generator/states-newstyle-opt-starttls.c
index d107b74..9eab023 100644
--- a/generator/states-newstyle-opt-starttls.c
+++ b/generator/states-newstyle-opt-starttls.c
@@ -23,7 +23,7 @@ STATE_MACHINE {
assert (h->gflags & LIBNBD_HANDSHAKE_FLAG_FIXED_NEWSTYLE);
/* If TLS was not requested we skip this option and go to the next one. */
if (h->tls == LIBNBD_TLS_DISABLE) {
- SET_NEXT_STATE (%^OPT_LIST.START);
+ SET_NEXT_STATE (%^OPT_STRUCTURED_REPLY.START);
return 0;
}
@@ -102,7 +102,7 @@ STATE_MACHINE {
debug (h,
"server refused TLS (%s), continuing with unencrypted connection",
reply == NBD_REP_ERR_POLICY ? "policy" : "not
supported");
- SET_NEXT_STATE (%^OPT_LIST.START);
+ SET_NEXT_STATE (%^OPT_STRUCTURED_REPLY.START);
return 0;
}
return 0;
@@ -121,7 +121,7 @@ STATE_MACHINE {
nbd_internal_crypto_debug_tls_enabled (h);
/* Continue with option negotiation. */
- SET_NEXT_STATE (%^OPT_LIST.START);
+ SET_NEXT_STATE (%^OPT_STRUCTURED_REPLY.START);
return 0;
}
/* Continue handshake. */
@@ -144,7 +144,7 @@ STATE_MACHINE {
debug (h, "connection is using TLS");
/* Continue with option negotiation. */
- SET_NEXT_STATE (%^OPT_LIST.START);
+ SET_NEXT_STATE (%^OPT_STRUCTURED_REPLY.START);
return 0;
}
/* Continue handshake. */
diff --git a/generator/states-newstyle.c b/generator/states-newstyle.c
index d771097..25ef351 100644
--- a/generator/states-newstyle.c
+++ b/generator/states-newstyle.c
@@ -124,6 +124,9 @@ STATE_MACHINE {
else
SET_NEXT_STATE (%OPT_SET_META_CONTEXT.START);
return 0;
+ case NBD_OPT_LIST:
+ SET_NEXT_STATE (%OPT_LIST.START);
+ return 0;
case NBD_OPT_ABORT:
if ((h->gflags & LIBNBD_HANDSHAKE_FLAG_FIXED_NEWSTYLE) == 0) {
SET_NEXT_STATE (%.DEAD);
diff --git a/lib/handle.c b/lib/handle.c
index a51b923..c308461 100644
--- a/lib/handle.c
+++ b/lib/handle.c
@@ -288,25 +288,11 @@ nbd_unlocked_get_export_description (struct nbd_handle *h)
return r;
}
-int
-nbd_unlocked_set_list_exports (struct nbd_handle *h, bool list)
-{
- h->list_exports = list;
- return 0;
-}
-
-/* NB: may_set_error = false. */
-int
-nbd_unlocked_get_list_exports (struct nbd_handle *h)
-{
- return h->list_exports;
-}
-
int
nbd_unlocked_get_nr_list_exports (struct nbd_handle *h)
{
- if (!h->list_exports) {
- set_error (EINVAL, "list exports mode not selected on this handle");
+ if (!h->exports) {
+ set_error (EINVAL, "nbd_opt_list not yet run on this handle");
return -1;
}
return (int) h->nr_exports;
@@ -318,8 +304,8 @@ nbd_unlocked_get_list_export_name (struct nbd_handle *h,
{
char *name;
- if (!h->list_exports) {
- set_error (EINVAL, "list exports mode not selected on this handle");
+ if (!h->exports) {
+ set_error (EINVAL, "nbd_opt_list not yet run on this handle");
return NULL;
}
if (i < 0 || i >= (int) h->nr_exports) {
@@ -340,8 +326,8 @@ nbd_unlocked_get_list_export_description (struct nbd_handle *h,
{
char *desc;
- if (!h->list_exports) {
- set_error (EINVAL, "list exports mode not selected on this handle");
+ if (!h->exports) {
+ set_error (EINVAL, "nbd_opt_list not yet run on this handle");
return NULL;
}
if (i < 0 || i >= (int) h->nr_exports) {
diff --git a/lib/opt.c b/lib/opt.c
index 6ad7f1a..7458b4b 100644
--- a/lib/opt.c
+++ b/lib/opt.c
@@ -78,6 +78,39 @@ nbd_unlocked_opt_abort (struct nbd_handle *h)
return wait_for_option (h);
}
+/* Issue NBD_OPT_LIST and wait for the reply. */
+int
+nbd_unlocked_opt_list (struct nbd_handle *h)
+{
+ size_t i;
+
+ if ((h->gflags & LIBNBD_HANDSHAKE_FLAG_FIXED_NEWSTYLE) == 0) {
+ set_error (ENOTSUP, "server is not using fixed newstyle protocol");
+ return -1;
+ }
+
+ /* Overwrite any previous results */
+ if (h->exports) {
+ for (i = 0; i < h->nr_exports; ++i) {
+ free (h->exports[i].name);
+ free (h->exports[i].description);
+ }
+ h->nr_exports = 0;
+ }
+ else {
+ h->exports = malloc (sizeof *h->exports);
+ if (h->exports == NULL) {
+ set_error (errno, "malloc");
+ return -1;
+ }
+ }
+
+ h->current_opt = NBD_OPT_LIST;
+ if (nbd_internal_run (h, cmd_issue) == -1)
+ debug (h, "option queued, ignoring state machine failure");
+ return wait_for_option (h);
+}
+
/* Issue NBD_OPT_GO (or NBD_OPT_EXPORT_NAME) without waiting. */
int
nbd_unlocked_aio_opt_go (struct nbd_handle *h)
diff --git a/tests/newstyle-limited.c b/tests/newstyle-limited.c
index aa6f87b..f2f1cba 100644
--- a/tests/newstyle-limited.c
+++ b/tests/newstyle-limited.c
@@ -136,7 +136,14 @@ main (int argc, char *argv[])
fprintf (stderr, "unexpected state after negotiating\n");
exit (EXIT_FAILURE);
}
- /* XXX Test nbd_opt_list when it is implemented */
+ if (nbd_opt_list (nbd) != -1) {
+ fprintf (stderr, "nbd_opt_list: expected failure\n");
+ exit (EXIT_FAILURE);
+ }
+ if (nbd_get_errno () != ENOTSUP) {
+ fprintf (stderr, "%s: wrong errno value after failed opt_list\n",
argv[0]);
+ exit (EXIT_FAILURE);
+ }
if (nbd_opt_abort (nbd) == -1) {
fprintf (stderr, "%s\n", nbd_get_error ());
exit (EXIT_FAILURE);
diff --git a/examples/list-exports.c b/examples/list-exports.c
index daea2ab..fcd474b 100644
--- a/examples/list-exports.c
+++ b/examples/list-exports.c
@@ -55,29 +55,31 @@ main (int argc, char *argv[])
exit (EXIT_FAILURE);
}
- /* Set opt mode and request list exports. */
+ /* Set opt mode. */
nbd_set_opt_mode (nbd, true);
- nbd_set_list_exports (nbd, true);
/* Connect to the NBD server over a
- * Unix domain socket. A side effect of
- * connecting is to list the exports.
- * This operation can fail normally, so
- * we need to check the return value and
- * error code.
+ * Unix domain socket. If we did not
+ * end up in option mode, then a
+ * listing is not possible.
*/
r = nbd_connect_unix (nbd, argv[1]);
- if (r == -1 && nbd_get_errno () == ENOTSUP) {
+ if (r == -1) {
fprintf (stderr, "%s\n", nbd_get_error ());
exit (EXIT_FAILURE);
}
- if (!nbd_aio_is_negotiating (nbd) ||
- nbd_get_nr_list_exports (nbd) == 0) {
+ if (!nbd_aio_is_negotiating (nbd)) {
fprintf (stderr, "Server does not support "
"listing exports.\n");
exit (EXIT_FAILURE);
}
+ /* Grab the export list. */
+ if (nbd_opt_list (nbd) == -1) {
+ fprintf (stderr, "%s\n", nbd_get_error ());
+ exit (EXIT_FAILURE);
+ }
+
/* Display the list of exports. */
for (i = 0;
i < nbd_get_nr_list_exports (nbd);
diff --git a/interop/list-exports.c b/interop/list-exports.c
index d003ce9..5af1234 100644
--- a/interop/list-exports.c
+++ b/interop/list-exports.c
@@ -52,8 +52,8 @@ main (int argc, char *argv[])
exit (EXIT_FAILURE);
}
- /* Set the list exports mode in the handle. */
- nbd_set_list_exports (nbd, true);
+ /* Set option mode in the handle. */
+ nbd_set_opt_mode (nbd, true);
/* Run qemu-nbd. */
char *args[] = { SERVER, SERVER_PARAMS, NULL };
@@ -62,9 +62,11 @@ main (int argc, char *argv[])
#else
#define NBD_CONNECT nbd_connect_command
#endif
- if (NBD_CONNECT (nbd, args) == -1)
- /* This is not an error so don't fail here. */
+ if (NBD_CONNECT (nbd, args) == -1 || nbd_opt_list (nbd) == -1) {
fprintf (stderr, "%s\n", nbd_get_error ());
+ unlink (tmpfile);
+ exit (EXIT_FAILURE);
+ }
/* We don't need the temporary file any longer. */
unlink (tmpfile);
@@ -98,6 +100,7 @@ main (int argc, char *argv[])
free (desc);
}
+ nbd_opt_abort (nbd);
nbd_close (nbd);
exit (EXIT_SUCCESS);
}
diff --git a/info/nbdinfo.c b/info/nbdinfo.c
index 394f5ac..cdc0db8 100644
--- a/info/nbdinfo.c
+++ b/info/nbdinfo.c
@@ -184,21 +184,27 @@ main (int argc, char *argv[])
}
nbd_set_uri_allow_local_file (nbd, true); /* Allow ?tls-psk-file. */
- /* If using --list then we set the list_exports mode flag in the
- * handle. In this case it can be OK for the connection to fail.
- * In particular it can fail because the export in the URI is not
- * recognized.
- */
+ /* If using --list then we need opt mode in the handle. */
if (list_all)
- nbd_set_list_exports (nbd, true);
+ nbd_set_opt_mode (nbd, true);
else if (!size_only)
nbd_set_full_info (nbd, true);
if (nbd_connect_uri (nbd, argv[optind]) == -1) {
- if (!list_all || nbd_get_errno () == ENOTSUP) {
+ fprintf (stderr, "%s\n", nbd_get_error ());
+ exit (EXIT_FAILURE);
+ }
+
+ if (list_all) {
+ if (nbd_opt_list (nbd) == -1) {
fprintf (stderr, "%s\n", nbd_get_error ());
exit (EXIT_FAILURE);
}
+ /* Disconnect from the server to move the handle into a closed
+ * state, in case the server serializes further connections.
+ * But we can ignore errors in this case.
+ */
+ nbd_opt_abort (nbd);
}
if (size_only) {
@@ -211,21 +217,10 @@ main (int argc, char *argv[])
printf ("%" PRIi64 "\n", size);
}
else {
- /* Print per-connection fields.
- *
- * XXX Not always displayed when using --list mode. This is
- * because if the connection fails above (as we expect) then the
- * handle state is dead and we cannot query these.
- */
+ /* Print per-connection fields. */
protocol = nbd_get_protocol (nbd);
tls_negotiated = nbd_get_tls_negotiated (nbd);
- /* Disconnect from the server to move the handle into a closed
- * state, in case the server serializes further connections; but
- * ignore errors as the connection may already be dead.
- */
- nbd_shutdown (nbd, 0);
-
if (!json_output) {
if (protocol) {
printf ("protocol: %s", protocol);
--
2.28.0