[Libguestfs] [libnbd PATCH 3/3] api: Add nbd_opt_list_meta_context
Richard W.M. Jones
rjones at redhat.com
Thu Oct 1 15:26:58 UTC 2020
On Mon, Sep 28, 2020 at 05:05:18PM -0500, Eric Blake wrote:
> Right now, we require the user to supply potential metacontext names
> in advance, without knowing what the server actually supports until
> after the connection or nbd_opt_info call is complete. But the NBD
> protocol also supports a client being able to query what contexts a
> server supports, including where a client asks with a prefix and the
> server replies back with all answers that match the prefix.
>
> Not to mention this will allow nbdinfo to gain full feature parity
> with 'qemu-nbd --list'.
I think I missed this review, sorry.
This feature looks great. I think you said in the
cover letter that you wanted to do further work, but I'll
ACK it anyway.
Rich.
> lib/internal.h | 1 +
> generator/API.ml | 84 +++++++++++++++++++-
> generator/states-newstyle-opt-meta-context.c | 80 +++++++++++++++----
> generator/states-newstyle.c | 3 +
> lib/opt.c | 73 +++++++++++++++++
> TODO | 11 +++
> 6 files changed, 234 insertions(+), 18 deletions(-)
>
> diff --git a/lib/internal.h b/lib/internal.h
> index cde5dcd..ad1eeb9 100644
> --- a/lib/internal.h
> +++ b/lib/internal.h
> @@ -77,6 +77,7 @@ struct command_cb {
> nbd_extent_callback extent;
> nbd_chunk_callback chunk;
> nbd_list_callback list;
> + nbd_context_callback context;
> } fn;
> nbd_completion_callback completion;
> };
> diff --git a/generator/API.ml b/generator/API.ml
> index 938ace4..358ec38 100644
> --- a/generator/API.ml
> +++ b/generator/API.ml
> @@ -142,8 +142,13 @@ let list_closure = {
> cbname = "list";
> cbargs = [ CBString "name"; CBString "description" ]
> }
> +let context_closure = {
> + cbname = "context";
> + cbargs = [ CBString "name" ]
> +}
> let all_closures = [ chunk_closure; completion_closure;
> - debug_closure; extent_closure; list_closure ]
> + debug_closure; extent_closure; list_closure;
> + context_closure ]
>
> (* Enums. *)
> let tls_enum = {
> @@ -852,7 +857,7 @@ 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_list";
> - Link "opt_info"];
> + Link "opt_info"; Link "opt_list_meta_context"];
> };
>
> "get_opt_mode", {
> @@ -960,6 +965,56 @@ corresponding L<nbd_opt_go(3)> would succeed.";
> Link "set_export_name"];
> };
>
> + "opt_list_meta_context", {
> + default_call with
> + args = [ Closure context_closure ]; ret = RInt;
> + permitted_states = [ Negotiating ];
> + shortdesc = "request the server to list available meta contexts";
> + longdesc = "\
> +Request that the server list available meta contexts associated with
> +the export previously specified by the most recent
> +L<nbd_set_export_name(3)> or L<nbd_connect_uri(3)>. This can only be
> +used if L<nbd_set_opt_mode(3)> enabled option mode.
> +
> +The NBD protocol allows a client to decide how many queries to ask
> +the server. Rather than taking that list of queries as a parameter
> +to this function, libnbd reuses the current list of requested meta
> +contexts as set by L<nbd_add_meta_context(3)>; you can use
> +L<nbd_clear_meta_contexts(3)> to set up a different list of queries.
> +When the list is empty, a server will typically reply with all
> +contexts that it supports; when the list is non-empty, the server
> +will reply only with supported contexts that match the client's
> +request. Note that a reply by the server might be encoded to
> +represent several feasible contexts on one string, rather than an
> +actual context name that would actually succeed during
> +L<nbd_opt_go(3)>; so it is still necessary to use
> +L<nbd_can_meta_context(3)> after connecting to see which contexts
> +are actually supported.
> +
> +The C<context> function is called once per server reply, with any
> +C<user_data> passed to this function, and with C<name> supplied by
> +the server. Remember that it is not safe to call
> +L<nbd_add_meta_context(3)> from within the context of the
> +callback function; rather, your code must copy any C<name> needed for
> +later use after this function completes. At present, the return value
> +of the callback is ignored, although a return of -1 should be avoided.
> +
> +For convenience, when this function succeeds, it returns the number
> +of replies returned by the server.
> +
> +Not all servers understand this request, and even when it is understood,
> +the server might intentionally send an empty list because it does not
> +support the requested context, or may encounter a failure after
> +delivering partial results. Thus, this function may succeed even when
> +no contexts 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
> +replies that might be advertised, so client code should be aware that
> +a server may send a lengthy list.";
> + see_also = [Link "set_opt_mode"; Link "aio_opt_list_meta_context";
> + Link "add_meta_context"; Link "clear_meta_contexts";
> + Link "opt_go"; Link "set_export_name"];
> + };
> +
> "add_meta_context", {
> default_call with
> args = [ String "name" ]; ret = RErr;
> @@ -2233,6 +2288,29 @@ callback.";
> see_also = [Link "set_opt_mode"; Link "opt_info"; Link "is_read_only"];
> };
>
> + "aio_opt_list_meta_context", {
> + default_call with
> + args = [ Closure context_closure ]; ret = RInt;
> + optargs = [ OClosure completion_closure ];
> + permitted_states = [ Negotiating ];
> + shortdesc = "request the server to list available meta contexts";
> + longdesc = "\
> +Request that the server list available meta contexts associated with
> +the export previously specified by the most recent
> +L<nbd_set_export_name(3)> or L<nbd_connect_uri(3)>. This can only be
> +used if L<nbd_set_opt_mode(3)> enabled option mode.
> +
> +To determine when the request completes, wait for
> +L<nbd_aio_is_connecting(3)> to return false. Or supply the optional
> +C<completion_callback> which will be invoked as described in
> +L<libnbd(3)/Completion callbacks>, except that it is automatically
> +retired regardless of return value. Note that detecting whether the
> +server returns an error (as is done by the return value of the
> +synchronous counterpart) is only possible with a completion
> +callback.";
> + see_also = [Link "set_opt_mode"; Link "opt_list_meta_context"];
> + };
> +
> "aio_pread", {
> default_call with
> args = [ BytesPersistOut ("buf", "count"); UInt64 "offset" ];
> @@ -2884,6 +2962,8 @@ let first_version = [
> "get_nr_meta_contexts", (1, 6);
> "get_meta_context", (1, 6);
> "clear_meta_contexts", (1, 6);
> + "opt_list_meta_context", (1, 6);
> + "aio_opt_list_meta_context", (1, 6);
>
> (* 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/states-newstyle-opt-meta-context.c b/generator/states-newstyle-opt-meta-context.c
> index 0dc48af..ce878ee 100644
> --- a/generator/states-newstyle-opt-meta-context.c
> +++ b/generator/states-newstyle-opt-meta-context.c
> @@ -21,18 +21,28 @@
> STATE_MACHINE {
> NEWSTYLE.OPT_META_CONTEXT.START:
> size_t i, nr_queries;
> - uint32_t len;
> + uint32_t len, opt;
>
> /* If the server doesn't support SRs then we must skip this group.
> * Also we skip the group if the client didn't request any metadata
> - * contexts.
> + * contexts, when doing SET (but an empty LIST is okay).
> */
> assert (h->gflags & LIBNBD_HANDSHAKE_FLAG_FIXED_NEWSTYLE);
> nbd_internal_reset_size_and_flags (h);
> - if (!h->structured_replies ||
> - nbd_internal_string_list_length (h->request_meta_contexts) == 0) {
> - SET_NEXT_STATE (%^OPT_GO.START);
> - return 0;
> + if (h->opt_current == NBD_OPT_LIST_META_CONTEXT) {
> + assert (h->opt_mode);
> + assert (h->structured_replies);
> + assert (CALLBACK_IS_NOT_NULL (h->opt_cb.fn.context));
> + opt = h->opt_current;
> + }
> + else {
> + assert (CALLBACK_IS_NULL (h->opt_cb.fn.context));
> + opt = NBD_OPT_SET_META_CONTEXT;
> + if (!h->structured_replies ||
> + nbd_internal_string_list_length (h->request_meta_contexts) == 0) {
> + SET_NEXT_STATE (%^OPT_GO.START);
> + return 0;
> + }
> }
>
> assert (h->meta_contexts == NULL);
> @@ -44,7 +54,7 @@ STATE_MACHINE {
> len += 4 /* length of query */ + strlen (h->request_meta_contexts[i]);
>
> h->sbuf.option.version = htobe64 (NBD_NEW_VERSION);
> - h->sbuf.option.option = htobe32 (NBD_OPT_SET_META_CONTEXT);
> + h->sbuf.option.option = htobe32 (opt);
> h->sbuf.option.optlen = htobe32 (len);
> h->wbuf = &h->sbuf;
> h->wlen = sizeof (h->sbuf.option);
> @@ -98,7 +108,8 @@ STATE_MACHINE {
> return 0;
>
> NEWSTYLE.OPT_META_CONTEXT.PREPARE_NEXT_QUERY:
> - const char *query = h->request_meta_contexts[h->querynum];
> + const char *query = !h->request_meta_contexts ? NULL
> + : h->request_meta_contexts[h->querynum];
>
> if (query == NULL) { /* end of list of requested meta contexts */
> SET_NEXT_STATE (%PREPARE_FOR_REPLY);
> @@ -140,10 +151,17 @@ STATE_MACHINE {
> return 0;
>
> NEWSTYLE.OPT_META_CONTEXT.RECV_REPLY:
> + uint32_t opt;
> +
> + if (h->opt_current == NBD_OPT_LIST_META_CONTEXT)
> + opt = h->opt_current;
> + else
> + opt = NBD_OPT_SET_META_CONTEXT;
> +
> switch (recv_into_rbuf (h)) {
> case -1: SET_NEXT_STATE (%.DEAD); return 0;
> case 0:
> - if (prepare_for_reply_payload (h, NBD_OPT_SET_META_CONTEXT) == -1) {
> + if (prepare_for_reply_payload (h, opt) == -1) {
> SET_NEXT_STATE (%.DEAD);
> return 0;
> }
> @@ -163,12 +181,25 @@ STATE_MACHINE {
> uint32_t len;
> const size_t maxpayload = sizeof h->sbuf.or.payload.context;
> struct meta_context *meta_context;
> + uint32_t opt;
> + int err = 0;
> +
> + if (h->opt_current == NBD_OPT_LIST_META_CONTEXT)
> + opt = h->opt_current;
> + else
> + opt = NBD_OPT_SET_META_CONTEXT;
>
> reply = be32toh (h->sbuf.or.option_reply.reply);
> len = be32toh (h->sbuf.or.option_reply.replylen);
> switch (reply) {
> case NBD_REP_ACK: /* End of list of replies. */
> - SET_NEXT_STATE (%^OPT_GO.START);
> + if (opt == NBD_OPT_LIST_META_CONTEXT) {
> + SET_NEXT_STATE (%.NEGOTIATING);
> + CALL_CALLBACK (h->opt_cb.completion, &err);
> + nbd_internal_free_option (h);
> + }
> + else
> + SET_NEXT_STATE (%^OPT_GO.START);
> break;
> case NBD_REP_META_CONTEXT: /* A context. */
> if (len > maxpayload)
> @@ -194,21 +225,38 @@ STATE_MACHINE {
> }
> debug (h, "negotiated %s with context ID %" PRIu32,
> meta_context->name, meta_context->context_id);
> - meta_context->next = h->meta_contexts;
> - h->meta_contexts = meta_context;
> + if (opt == NBD_OPT_LIST_META_CONTEXT) {
> + CALL_CALLBACK (h->opt_cb.fn.context, meta_context->name);
> + free (meta_context->name);
> + free (meta_context);
> + }
> + else {
> + meta_context->next = h->meta_contexts;
> + h->meta_contexts = meta_context;
> + }
> }
> SET_NEXT_STATE (%PREPARE_FOR_REPLY);
> break;
> default:
> - /* Anything else is an error, ignore it */
> + /* Anything else is an error, ignore it for SET, report it for LIST */
> if (handle_reply_error (h) == -1) {
> SET_NEXT_STATE (%.DEAD);
> return 0;
> }
>
> - debug (h, "handshake: unexpected error from "
> - "NBD_OPT_SET_META_CONTEXT (%" PRIu32 ")", reply);
> - SET_NEXT_STATE (%^OPT_GO.START);
> + if (opt == NBD_OPT_LIST_META_CONTEXT) {
> + err = ENOTSUP;
> + set_error (err, "unexpected response, possibly the server does not "
> + "support listing contexts");
> + CALL_CALLBACK (h->opt_cb.completion, &err);
> + nbd_internal_free_option (h);
> + SET_NEXT_STATE (%.NEGOTIATING);
> + }
> + else {
> + debug (h, "handshake: unexpected error from "
> + "NBD_OPT_SET_META_CONTEXT (%" PRIu32 ")", reply);
> + SET_NEXT_STATE (%^OPT_GO.START);
> + }
> break;
> }
> return 0;
> diff --git a/generator/states-newstyle.c b/generator/states-newstyle.c
> index a0a5928..6fb7548 100644
> --- a/generator/states-newstyle.c
> +++ b/generator/states-newstyle.c
> @@ -136,6 +136,9 @@ STATE_MACHINE {
> }
> SET_NEXT_STATE (%PREPARE_OPT_ABORT);
> return 0;
> + case NBD_OPT_LIST_META_CONTEXT:
> + SET_NEXT_STATE (%OPT_META_CONTEXT.START);
> + return 0;
> case 0:
> break;
> default:
> diff --git a/lib/opt.c b/lib/opt.c
> index 6ea8326..2317b72 100644
> --- a/lib/opt.c
> +++ b/lib/opt.c
> @@ -32,6 +32,8 @@ nbd_internal_free_option (struct nbd_handle *h)
> {
> if (h->opt_current == NBD_OPT_LIST)
> FREE_CALLBACK (h->opt_cb.fn.list);
> + else if (h->opt_current == NBD_OPT_LIST_META_CONTEXT)
> + FREE_CALLBACK (h->opt_cb.fn.context);
> FREE_CALLBACK (h->opt_cb.completion);
> }
>
> @@ -166,6 +168,51 @@ nbd_unlocked_opt_list (struct nbd_handle *h, nbd_list_callback *list)
> return s.count;
> }
>
> +struct context_helper {
> + int count;
> + nbd_context_callback context;
> + int err;
> +};
> +static int
> +context_visitor (void *opaque, const char *name)
> +{
> + struct context_helper *h = opaque;
> + if (h->count < INT_MAX)
> + h->count++;
> + CALL_CALLBACK (h->context, name);
> + return 0;
> +}
> +static int
> +context_complete (void *opaque, int *err)
> +{
> + struct context_helper *h = opaque;
> + h->err = *err;
> + FREE_CALLBACK (h->context);
> + return 0;
> +}
> +
> +/* Issue NBD_OPT_LIST_META_CONTEXT and wait for the reply. */
> +int
> +nbd_unlocked_opt_list_meta_context (struct nbd_handle *h,
> + nbd_context_callback *context)
> +{
> + struct context_helper s = { .context = *context };
> + nbd_context_callback l = { .callback = context_visitor, .user_data = &s };
> + nbd_completion_callback c = { .callback = context_complete, .user_data = &s };
> +
> + if (nbd_unlocked_aio_opt_list_meta_context (h, &l, &c) == -1)
> + return -1;
> +
> + SET_CALLBACK_TO_NULL (*context);
> + if (wait_for_option (h) == -1)
> + return -1;
> + if (s.err) {
> + set_error (s.err, "server replied with error to list meta context request");
> + return -1;
> + }
> + return s.count;
> +}
> +
> /* Issue NBD_OPT_GO (or NBD_OPT_EXPORT_NAME) without waiting. */
> int
> nbd_unlocked_aio_opt_go (struct nbd_handle *h,
> @@ -230,3 +277,29 @@ nbd_unlocked_aio_opt_list (struct nbd_handle *h, nbd_list_callback *list,
> debug (h, "option queued, ignoring state machine failure");
> return 0;
> }
> +
> +/* Issue NBD_OPT_LIST_META_CONTEXT without waiting. */
> +int
> +nbd_unlocked_aio_opt_list_meta_context (struct nbd_handle *h,
> + nbd_context_callback *context,
> + nbd_completion_callback *complete)
> +{
> + if ((h->gflags & LIBNBD_HANDSHAKE_FLAG_FIXED_NEWSTYLE) == 0) {
> + set_error (ENOTSUP, "server is not using fixed newstyle protocol");
> + return -1;
> + }
> + if (!h->structured_replies) {
> + set_error (ENOTSUP, "server lacks structured replies");
> + return -1;
> + }
> +
> + assert (CALLBACK_IS_NULL (h->opt_cb.fn.context));
> + h->opt_cb.fn.context = *context;
> + SET_CALLBACK_TO_NULL (*context);
> + h->opt_cb.completion = *complete;
> + SET_CALLBACK_TO_NULL (*complete);
> + h->opt_current = NBD_OPT_LIST_META_CONTEXT;
> + if (nbd_internal_run (h, cmd_issue) == -1)
> + debug (h, "option queued, ignoring state machine failure");
> + return 0;
> +}
> diff --git a/TODO b/TODO
> index 4a0cd22..b6c676b 100644
> --- a/TODO
> +++ b/TODO
> @@ -13,6 +13,17 @@ NBD resize extension.
>
> TLS should properly shut down the session (calling gnutls_bye).
>
> +Potential deadlock with nbd_add_meta_context: if a client sends enough
> +requests to the server that it blocks while writing, but the server
> +replies to requests as they come in rather than waiting for the end of
> +the client request, then the server can likewise block in writing
> +replies that libnbd is not yet reading. Not an issue for existing
> +servers that don't have enough contexts to reply with enough data to
> +fill buffers, but could be an issue with qemu-nbd if it is taught to
> +exports many dirty bitmaps simultaneously. Revamping the
> +states-newstyle-meta-context.c state machine to let libnbd handle
> +NBD_REP_META_CONTEXT while still writing queries could be hairy.
> +
> Performance: Chart it over various buffer sizes and threads, as that
> should make it easier to identify systematic issues.
>
> --
> 2.28.0
>
> _______________________________________________
> Libguestfs mailing list
> Libguestfs at 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
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
More information about the Libguestfs
mailing list