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@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