[Libguestfs] [PATCH libnbd PROPOSAL] Add APIs for listing exports from an NBD server.
Eric Blake
eblake at redhat.com
Mon Jul 20 15:50:51 UTC 2020
On 7/20/20 9:43 AM, Richard W.M. Jones wrote:
> A major missing feature of this library was the ability to list
> exports from an NBD server. This implements the feature by adding a
> new handle mode and additional functions for querying the list of
> export names.
> ---
> .gitignore | 1 +
> examples/Makefile.am | 14 +++
> examples/list-exports.c | 108 +++++++++++++++++++
> generator/API.ml | 72 +++++++++++++
> generator/Makefile.am | 1 +
> generator/state_machine.ml | 39 +++++++
> generator/states-newstyle-opt-list.c | 131 +++++++++++++++++++++++
> generator/states-newstyle-opt-starttls.c | 8 +-
> lib/handle.c | 50 +++++++++
> lib/internal.h | 9 ++
> lib/nbd-protocol.h | 6 ++
> 11 files changed, 435 insertions(+), 4 deletions(-)
>
> +++ b/examples/list-exports.c
> @@ -0,0 +1,108 @@
> +/* This example shows how to list NBD exports.
> + *
> + * To test this with qemu-nbd:
> + * $ qemu-nbd -x "hello" -t -k /tmp/sock disk.img
> + * $ ./run examples/list-exports /tmp/sock
> + * [0] hello
> + * Which export to connect to? 0
> + * Connecting to hello ...
> + * /tmp/sock: hello: size = 2048 bytes
> + */
It's possible to use qemu-kvm to set up multiple export names (via QMP
commands) over one NBD server, if that would make the example any more
powerful (although it is a lot more verbose than the one-liner of
qemu-nbd which always has a single export).
And I'm guessing you are hoping to add counterpart APIs into nbdkit to
expose multiple exports (such as the tar or ext2 filter being able to
list available exports when using the client exportname...)
> + /* Set the list exports mode in the handle. */
> + nbd_set_list_exports (nbd, 1);
s/1/true/, given that our C bindings actually use the bool type.
> +
> + /* 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.
> + */
> + r = nbd_connect_unix (nbd, argv[1]);
> + if (r == -1 && nbd_get_errno () == ENOTSUP) {
> + fprintf (stderr, "%s\n", nbd_get_error ());
> + exit (EXIT_FAILURE);
> + }
> +
> + if (nbd_get_nr_list_exports (nbd) == 0) {
> + fprintf (stderr, "Server does not support "
> + "listing exports.\n");
> + exit (EXIT_FAILURE);
> + }
> +
> + /* Display the list of exports. */
> + for (i = 0;
> + i < nbd_get_nr_list_exports (nbd);
> + i++) {
> + name = nbd_get_list_export_name (nbd, i);
> + printf ("[%d] %s\n", i, name);
> + free (name);
> + }
Looks like a pretty simple way to do the iteration.
> + printf ("Which export to connect to? ");
> + if (scanf ("%d", &i) != 1) exit (EXIT_FAILURE);
scanf("%d") is undefined on integer overflow, but is simple enough for
use in this example. In fact, for an example I might just use atoi(),
with even worse error-handling behavior but easier use.
> + name = nbd_get_list_export_name (nbd, i);
> + printf ("Connecting to %s ...\n", name);
> + nbd_close (nbd);
> +
> + /* Connect again to the chosen export. */
> + nbd2 = nbd_create ();
> + if (nbd2 == NULL) {
> + 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);
> + }
I see what you did. Rather than letting a user re-use the connection
(and pick an export name after obtaining the listing), you require the
user to transfer the state of which export name is interesting from the
first handle in list mode over to the requested export in the second
handle. I don't see any convenient way to do it all on one handle, so
your approach is good enough.
> +++ b/generator/API.ml
> @@ -588,6 +588,72 @@ the time of compilation.";
> Link "aio_is_created"; Link "aio_is_ready"];
> };
>
> + "set_list_exports", {
> + default_call with
> + args = [Bool "list"]; ret = RErr;
> + permitted_states = [ Created ];
> + shortdesc = "set whether to list server exports";
> + longdesc = "\
> +Set this flag to true on a handle in order to list NBD exports
> +provided by the server.
> +
> +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
> +C<nbd_get_nr_exports> and C<nbd_get_export_name>. After choosing
> +the export you want, you should close this handle, create a new
> +NBD handle (C<nbd_create>), set the export name (C<nbd_set_export_name>),
> +and connect on the new handle.
> +
> +Some servers do not support listing exports at all. In
> +that case the connect call will return error C<ENOTSUP>
> +and C<nbd_get_nr_exports> 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 non-TLS or TLS connection is
> +opened.";
Here's looking at 'nbdkit info' which supports more exports than there
are atoms in the universe ;)
> + example = Some "examples/list-exports.c";
> + see_also = [Link "get_list_exports";
> + Link "get_nr_list_exports"; Link "get_list_export_name"];
> + };
Looks nice.
> +
> + "get_nr_list_exports", {
> + default_call with
> + args = []; ret = RInt;
> + permitted_states = [ 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
> +C<nbd_set_list_exports>.";
> + see_also = [Link "set_list_exports"];
> + };
> +
> + "get_list_export_name", {
> + default_call with
> + args = [ Int "i" ]; ret = RString;
> + permitted_states = [ 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
> +from the list returned by the server.";
> + see_also = [Link "set_list_exports"];
> + };
Is it worth trying to have some sort of a callback mode where the
callback function is called once per export name advertised while the
connection attempt is underway, rather than this post-mortem query of
the data copied into the nbd handle? But as your added example code
showed, this wasn't too bad to code with.
> +++ b/generator/state_machine.ml
> @@ -273,6 +273,7 @@ and newstyle_state_machine = [
> * state needs to run and skip to the next state in the list if not.
> *)
> 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);
> @@ -341,6 +342,44 @@ and newstyle_opt_starttls_state_machine = [
> };
> ]
>
> +(* Fixed newstyle NBD_OPT_LIST option. *)
> +and newstyle_opt_list_state_machine = [
> + State {
> + default_state with
> + name = "START";
> + comment = "Start listing exports if in list mode.";
> + external_events = [];
> + };
> +
> + State {
> + default_state with
> + name = "SEND";
> + comment = "Send newstyle NBD_OPT_LIST to being listing exports";
s/being/begin/
> +++ b/generator/states-newstyle-opt-list.c
> +STATE_MACHINE {
> + NEWSTYLE.OPT_LIST.START:
> + if (!h->list_exports) {
> + SET_NEXT_STATE (%^OPT_STRUCTURED_REPLY.START);
> + return 0;
> + }
> +
> + h->sbuf.option.version = htobe64 (NBD_NEW_VERSION);
> + h->sbuf.option.option = htobe32 (NBD_OPT_LIST);
> + h->sbuf.option.optlen = 0;
> + h->wbuf = &h->sbuf;
> + h->wlen = sizeof (h->sbuf.option);
> + SET_NEXT_STATE (%SEND);
> + return 0;
Is it worth enhancing the code to add another list mode where we can
also query NBD_OPT_INFO on each export thus listed? (Compare to
'qemu-nbd --list'). Hmm, if we do that, list mode may need to be an
enum instead of a bool. But that would be a followup because it adds
more complexity; this is already a good addition on its own.
> + NEWSTYLE.OPT_LIST.CHECK_REPLY:
> + const size_t maxpayload = sizeof h->sbuf.or.payload.server;
> + uint32_t reply;
> + uint32_t len;
> + uint32_t elen;
> + char *name;
> + char **new_exports;
> +
> + reply = be32toh (h->sbuf.or.option_reply.reply);
> + len = be32toh (h->sbuf.or.option_reply.replylen);
> + switch (reply) {
> + case NBD_REP_SERVER:
> + /* Got one export. */
> + if (len > maxpayload)
> + debug (h, "skipping too large export name reply");
> + else {
> + elen = be32toh (h->sbuf.or.payload.server.server.export_name_len);
> + if (elen > len - 4) {
> + set_error (0, "invalid export length");
> + SET_NEXT_STATE (%.DEAD);
> + return 0;
> + }
> + /* Copy the export name to the handle list. */
> + name = strndup (h->sbuf.or.payload.server.str, elen);
> + if (name == NULL) {
> + set_error (errno, "strdup");
> + SET_NEXT_STATE (%.DEAD);
> + return 0;
> + }
> + new_exports = realloc (h->exports, sizeof (char *) * (h->nr_exports+1));
> + if (new_exports == NULL) {
> + set_error (errno, "strdup");
> + SET_NEXT_STATE (%.DEAD);
> + free (name);
> + return 0;
> + }
> + h->exports = new_exports;
> + h->exports[h->nr_exports++] = name;
> + }
> +
> + /* Wait for more replies. */
> + h->rbuf = &h->sbuf;
> + h->rlen = sizeof (h->sbuf.or.option_reply);
> + SET_NEXT_STATE (%RECV_REPLY);
> + return 0;
> +
> + case NBD_REP_ACK:
> + /* Finished receiving the list. */
> + SET_NEXT_STATE (%^OPT_STRUCTURED_REPLY.START);
> + return 0;
> +
> + default:
> + if (handle_reply_error (h) == -1) {
> + SET_NEXT_STATE (%.DEAD);
Would this be better as %.CLOSED, or even a transition into a state
where we send a clean NBD_OPT_ABORT for graceful shutdown rather than
abrupt disconnect from the server?
> + return 0;
> + }
> + set_error (ENOTSUP, "unexpected response, possibly the server does not "
> + "support listing exports");
> + SET_NEXT_STATE (%.DEAD);
whereas this definitely makes sense as %.DEAD.
> + return 0;
> + }
> + return 0;
> +
> +} /* END STATE MACHINE */
> diff --git a/generator/states-newstyle-opt-starttls.c b/generator/states-newstyle-opt-starttls.c
> index d220c4f..2d74e5f 100644
> --- a/generator/states-newstyle-opt-starttls.c
> +++ b/lib/handle.c
> +int
> +nbd_unlocked_set_list_exports (struct nbd_handle *h, bool list)
> +{
> + h->list_exports = true;
s/true/list/ (you never really tested clearing the mode...)
> +char *
> +nbd_unlocked_get_list_export_name (struct nbd_handle *h,
> + int i)
> +{
> + char *name;
> +
> + if (!h->list_exports) {
> + set_error (EINVAL, "list exports mode not selected on this handle");
> + return NULL;
> + }
> + if (i < 0 || i >= (int) h->nr_exports) {
That cast makes sense, but looks odd - any server that tries to send
more than 2G export names (by assuming that our size_t > 32 bits) is
sending a LOT of traffic in response to a single NBD_OPT_LIST command.
Would it be better to just track the list size as int, and change the
state machine to fail if the server hasn't completed its listing in X
number of responses (perhaps where X is 1 million, or determined by the
user, or...)?
> +++ b/lib/nbd-protocol.h
> @@ -154,6 +154,12 @@ struct nbd_fixed_new_option_reply_info_export {
> uint16_t eflags; /* per-export flags */
> } NBD_ATTRIBUTE_PACKED;
>
> +/* NBD_REP_SERVER reply (follows fixed_new_option_reply). */
> +struct nbd_fixed_new_option_reply_server {
> + uint32_t export_name_len; /* length of export name */
> + /* followed by a string export name and description*/
> +} NBD_ATTRIBUTE_PACKED;
> +
> /* NBD_REP_META_CONTEXT reply (follows fixed_new_option_reply). */
> struct nbd_fixed_new_option_reply_meta_context {
> uint32_t context_id; /* metadata context ID */
>
We'll want this synchronized with nbdkit.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
More information about the Libguestfs
mailing list