[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Libguestfs] [libnbd PATCH 3/3] api: Add nbd_opt_list_meta_context



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]