Re: [Libguestfs] [nbdkit PATCH v2 2/8] api: Add nbdkit_add_default_export

Richard W.M. Jones
2020-08-27T07:57:35Z

On Wed, Aug 26, 2020 at 09:16:46PM -0500, Eric Blake
wrote:
> Rather than defaulting .list_exports to blindly returning "", it
is
> nicer to have it reflect the result of .default_export.  Meanwhile,
> language bindings will have a C callback for both .list_exports and
> .default_export, even if the underlying script does not, which means
> any logic in plugins.c for calling .default_export when .list_export
> is missing would have to be duplicated in each language binding.
> Better is to make it easy for languages, by wrapping things in a new
> helper function; this also lets us take maximum advantage of caching
> done in backend.c (since language bindings can't access our internals,
> they would have to duplicate caching if we did not add this helper
> function).

I'm a bit confused by how nbdkit_add_default_export() should be used
and the documentation additions in this patch definitely need some
work, but I'm going to assume it's used like this:

  my_list_exports (exports)
  {
    nbdkit_add_export (exports, "", some_description);
    /* Mark the most recently added export as the default: */
    nbdkit_add_default_export (exports);

    /* Some other exports: */
    nbdkit_add_export (exports, "foo", NULL);
    nbdkit_add_export (exports, "bar", NULL);
  }

You could then argue that .default_export should call
nbdkit_add_export + nbdkit_add_default_export instead of having to
deal with all the interning stuff ...  What do you think about that
change?

  my_default_export (exports)
  {
    /* nbdkit_add_export should only be called once. */
    nbdkit_add_export (exports, "", some_description);
    nbdkit_add_default_export (exports);
  }

But then again this is differently awkward to consume from the server
side.

And if we did this I can't really see why we need the .default_export
callback at all.  The server could just call .list_exports early on
and keep track internally of the exports and default export name that
the plugin advertises.

Rich.

> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  docs/nbdkit-plugin.pod  | 21 +++++++++++++--------
>  include/nbdkit-common.h |  2 ++
>  server/internal.h       |  4 ++++
>  server/backend.c        | 15 ++++++++-------
>  server/exports.c        | 24 ++++++++++++++++++++++++
>  server/nbdkit.syms      |  1 +
>  server/plugins.c        |  2 +-
>  server/test-public.c    |  9 ++++++++-
>  plugins/sh/methods.c    |  2 +-
>  tests/test-layers.c     | 12 ++++++++++++
>  10 files changed, 74 insertions(+), 18 deletions(-)
>
> diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod
> index 1f208b27..50cf991d 100644
> --- a/docs/nbdkit-plugin.pod
> +++ b/docs/nbdkit-plugin.pod
> @@ -675,10 +675,11 @@ error message and return C<-1>.
>
>  This optional callback is called if the client tries to list the
>  exports served by the plugin (using C<NBD_OPT_LIST>).  If the plugin
> -does not supply this callback then a single export called
C<""> is
> -returned.  The NBD protocol defines C<""> as the default
export, so
> -this is suitable for plugins which ignore the export name and always
> -serve the same content.  See also L</EXPORT NAME> below.
> +does not supply this callback then the result of C<.default_export>
is
> +advertised as the lone export.  The NBD protocol defines
C<""> as the
> +default export, so this is suitable for plugins which ignore the
> +export name and always serve the same content.  See also L</EXPORT
> +NAME> below.
>
>  The C<readonly> flag informs the plugin that the server was started
>  with the I<-r> flag on the command line, which is the same value
> @@ -694,12 +695,13 @@ C<"">).  The plugin can ignore this
flag and return all exports if it
>  wants.
>
>  The C<exports> parameter is an opaque object for collecting the list
> -of exports.  Call C<nbdkit_add_export> to add a single export to the
> -list.  If the plugin has a concept of a default export (usually but
> -not always called C<"">) then it should return that first
in the list.
> +of exports.  Call C<nbdkit_add_export> as needed to add specific
> +exports to the list, or C<nbdkit_add_default_export> to add a single
> +entry based on the results of C<.default_export>.
>
>   int nbdkit_add_export (struct nbdkit_export *exports,
>                          const char *name, const char *description);
> + int nbdkit_add_default_export (struct nbdkit_export *exports);
>
>  The C<name> must be a non-NULL, UTF-8 string between 0 and 4096
bytes
>  in length.  Export names must be unique.  C<description> is an
> @@ -725,7 +727,10 @@ default export C<"">, where the plugin
provides a UTF-8 string between
>  connection continues with the empty name; if the plugin returns a
>  valid string, nbdkit returns that name to clients that support it (see
>  the C<NBD_INFO_NAME> response to C<NBD_OPT_GO>), and behaves
as if the
> -client had passed that string instead of an empty name.
> +client had passed that string instead of an empty name.  Similarly, if
> +the plugin does not supply a C<.list_exports> callback, the result
of
> +this callback determines what export name to advertise to a client
> +requesting an export list.
>
>  The C<readonly> flag informs the plugin that the server was started
>  with the I<-r> flag on the command line, which is the same value
> diff --git a/include/nbdkit-common.h b/include/nbdkit-common.h
> index c377e18d..5e8c0b6f 100644
> --- a/include/nbdkit-common.h
> +++ b/include/nbdkit-common.h
> @@ -130,6 +130,8 @@ struct nbdkit_exports;
>  NBDKIT_EXTERN_DECL (int, nbdkit_add_export,
>                      (struct nbdkit_exports *,
>                       const char *name, const char *description));
> +NBDKIT_EXTERN_DECL (int, nbdkit_add_default_export,
> +                    (struct nbdkit_exports *));
>
>  /* A static non-NULL pointer which can be used when you don't need a
>   * per-connection handle.
> diff --git a/server/internal.h b/server/internal.h
> index 8c8448e6..e2a68513 100644
> --- a/server/internal.h
> +++ b/server/internal.h
> @@ -543,4 +543,8 @@ extern struct connection *threadlocal_get_conn (void);
>    struct connection *conn = threadlocal_get_conn ();    \
>    assert (conn != NULL)
>
> +/* exports.c */
> +extern int exports_resolve_default (struct nbdkit_exports *exports,
> +                                    struct backend *b, int readonly);
> +
>  #endif /* NBDKIT_INTERNAL_H */
> diff --git a/server/backend.c b/server/backend.c
> index 00e65e3c..3e0a1d24 100644
> --- a/server/backend.c
> +++ b/server/backend.c
> @@ -164,7 +164,7 @@ backend_list_exports (struct backend *b, int readonly,
int default_only,
>  {
>    GET_CONN;
>    struct handle *h = get_handle (conn, b->i);
> -  int r;
> +  size_t count;
>
>    assert (!default_only); /* XXX Switch to is_tls... */
>    controlpath_debug ("%s: list_exports readonly=%d
default_only=%d",
> @@ -173,14 +173,15 @@ backend_list_exports (struct backend *b, int
readonly, int default_only,
>    assert (h->handle == NULL);
>    assert ((h->state & HANDLE_OPEN) == 0);
>
> -  r = b->list_exports (b, readonly, default_only, exports);
> -  if (r == -1)
> +  if (b->list_exports (b, readonly, default_only, exports) == -1 ||
> +      exports_resolve_default (exports, b, readonly) == -1) {
>      controlpath_debug ("%s: list_exports failed", b->name);
> -  else {
> -    size_t count = nbdkit_exports_count (exports);
> -    controlpath_debug ("%s: list_exports returned %zu names",
b->name, count);
> +    return -1;
>    }
> -  return r;
> +
> +  count = nbdkit_exports_count (exports);
> +  controlpath_debug ("%s: list_exports returned %zu names",
b->name, count);
> +  return 0;
>  }
>
>  const char *
> diff --git a/server/exports.c b/server/exports.c
> index 8d3faec4..3259fbdb 100644
> --- a/server/exports.c
> +++ b/server/exports.c
> @@ -52,6 +52,7 @@ DEFINE_VECTOR_TYPE(exports, struct nbdkit_export);
>
>  struct nbdkit_exports {
>    exports exports;
> +  bool use_default;
>  };
>
>  struct nbdkit_exports *
> @@ -65,6 +66,7 @@ nbdkit_exports_new (void)
>      return NULL;
>    }
>    r->exports = (exports) empty_vector;
> +  r->use_default = false;
>    return r;
>  }
>
> @@ -141,3 +143,25 @@ nbdkit_add_export (struct nbdkit_exports *exps,
>
>    return 0;
>  }
> +
> +int
> +nbdkit_add_default_export (struct nbdkit_exports *exps)
> +{
> +  exps->use_default = true;
> +  return 0;
> +}
> +
> +int
> +exports_resolve_default (struct nbdkit_exports *exps, struct backend *b,
> +                         int readonly)
> +{
> +  const char *def = NULL;
> +
> +  if (exps->use_default) {
> +    def = backend_default_export (b, readonly);
> +    exps->use_default = false;
> +  }
> +  if (def)
> +    return nbdkit_add_export (exps, def, NULL);
> +  return 0;
> +}
> diff --git a/server/nbdkit.syms b/server/nbdkit.syms
> index a67669b7..212e36aa 100644
> --- a/server/nbdkit.syms
> +++ b/server/nbdkit.syms
> @@ -39,6 +39,7 @@
>    # The functions we want plugins and filters to call.
>    global:
>      nbdkit_absolute_path;
> +    nbdkit_add_default_export;
>      nbdkit_add_export;
>      nbdkit_add_extent;
>      nbdkit_debug;
> diff --git a/server/plugins.c b/server/plugins.c
> index 8172759f..da28b2f1 100644
> --- a/server/plugins.c
> +++ b/server/plugins.c
> @@ -289,7 +289,7 @@ plugin_list_exports (struct backend *b, int readonly,
int default_only,
>    struct backend_plugin *p = container_of (b, struct backend_plugin,
backend);
>
>    if (!p->plugin.list_exports)
> -    return nbdkit_add_export (exports, "", NULL);
> +    return nbdkit_add_default_export (exports);
>
>    return p->plugin.list_exports (readonly, default_only, exports);
>  }
> diff --git a/server/test-public.c b/server/test-public.c
> index 0149dd9b..ee71f2fc 100644
> --- a/server/test-public.c
> +++ b/server/test-public.c
> @@ -66,7 +66,14 @@ threadlocal_get_conn (void)
>    abort ();
>  }
>
> -int connection_get_status (void)
> +int
> +connection_get_status (void)
> +{
> +  abort ();
> +}
> +
> +const char *
> +backend_default_export (struct backend *b, int readonly)
>  {
>    abort ();
>  }
> diff --git a/plugins/sh/methods.c b/plugins/sh/methods.c
> index 8e2e4256..2192ae25 100644
> --- a/plugins/sh/methods.c
> +++ b/plugins/sh/methods.c
> @@ -316,7 +316,7 @@ sh_list_exports (int readonly, int default_only,
>      return parse_exports (script, s, slen, exports);
>
>    case MISSING:
> -    return nbdkit_add_export (exports, "", NULL);
> +    return nbdkit_add_default_export (exports);
>
>    case ERROR:
>      return -1;
> diff --git a/tests/test-layers.c b/tests/test-layers.c
> index db0a2da0..fa7730e6 100644
> --- a/tests/test-layers.c
> +++ b/tests/test-layers.c
> @@ -331,6 +331,18 @@ main (int argc, char *argv[])
>     * than open-coding a naive client.
>     */
>
> +  /* .default_export methods called in outer-to-inner order. */
> +  log_verify_seen_in_order
> +    ("testlayersfilter3: default_export",
> +     "filter3: test_layers_filter_default_export",
> +     "testlayersfilter2: default_export",
> +     "filter2: test_layers_filter_default_export",
> +     "testlayersfilter1: default_export",
> +     "filter1: test_layers_filter_default_export",
> +     "testlayersplugin: default_export",
> +     "test_layers_plugin_default_export",
> +     NULL);
> +
>    /* open methods called in outer-to-inner order, but thanks to next
>     * pointer, complete in inner-to-outer order. */
>    log_verify_seen_in_order
> --
> 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
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org