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

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

On 8/27/20 2:57 AM, Richard W.M. Jones wrote:
> 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);
>    }
Yes, we have a documentation hole, because that's not the use case I had 
in mind.  Looking at patch 8 for how the exportname filter uses it may 
make more sense.

My intent is that 'nbdkit_add_default_export(exports)' behaves as 
short-hand for 'nbdkit_add_export(exports, default_export(), NULL)' but 
with less boilerplate and more convenience (it better handles when 
default_export() returns NULL, and lets nbdkit cache things so a later 
call to default_export can reuse the cache).  The intended usage is in 
language bindings, which can do:

glue_default_export (readonly, is_tls)
{
   if (real_default_export)
     return real_default_export(readonly, is_tls)
   else
     return "";
}

glue_list_exports (readonly, is_tls, exports)
{
   if (real_list_exports)
     return real_list_exports (readonly, is_tls, exports);
   else
     return nbdkit_add_default_export (exports);
}

rather than this (with glue_default_export unchanged):

glue_list_exports (readonly, is_tls, exports)
{
   if (real_list_exports)
     return real_list_exports (readonly, is_tls, exports);
   else {
     // unsafe: if glue_default_export() gives NULL, .list_exports fails
     // return nbdkit_add_export (exports, glue_default_export(readonly,
     //                                        is_tls), NULL);
     const char *def = glue_default_export(readonly, is_tls);
     if (def)
       return nbdkit_add_export (exports, def, NULL);
     return 0;
   }
}

> 
> 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?
Awkward.  As I designed things, there are four different client paths 
for triggering our calls:

client calls NBD_OPT_GO("a") - neither .list_exports nor
.default_export
is needed

client calls NBD_OPT_GO("") - .list_exports is not needed, but 
.default_export is

client calls NBD_OPT_LIST, then NBD_OPT_GO("") - both .list_exports
and
.default_export are needed; and we prefer to cache things so that the 
plugin's .default_export is called exactly once (if glue_list_exports 
calls glue_default_export, then we've lost that caching effect; but 
letting glue_list_exports use nbdkit_add_default_export() solves it)

client calls NBD_OPT_LIST, then NBD_OPT_GO("a") - .list_exports is 
needed. Whether .default_export is called or not depends on the plugin; 
if the plugin is happy with listing all things explicitly then 
.default_export can be skipped; if the plugin omits .list_exports then 
calling .default_export provides a saner list than [""]

My worry is that .list_exports has some overhead - it can be expensive 
to collect a list of exports and store them in memory on every 
connection, particularly when many clients do NOT care about the list. 
Yes, we could state that the nbdkit driver _always_ calls .list_exports 
after .preconnect for each new client, at which point we don't need a 
separate .default_export (but instead have a way during .list_exports to 
mark _which_ export to use as default, if any).  But it will be more 
common to have a client that needs .default_export than a client that 
needs .list_exports, so allowing .default_export to be easy and fast, 
and declaring that plugins cannot rely on .list_exports being called, is 
worthwhile.

> 
>    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);
So I definitely need to respin the doc portion of the patch.

In my usage, I never mixed add_export and add_default_export in the same 
control path (it was one or the other), but I also didn't see a problem 
with a plugin that wants to include its default name in addition to 
other names.

Would bike-shedding the name help, maybe nbdkit_use_default_export, to 
make it obvious that this is instructing nbdkit to reuse .default_export?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On Thu, Aug 27, 2020 at 07:55:48AM -0500, Eric Blake
wrote:
> So I definitely need to respin the doc portion of the patch.
> 
> In my usage, I never mixed add_export and add_default_export in the
> same control path (it was one or the other), but I also didn't see a
> problem with a plugin that wants to include its default name in
> addition to other names.
> 
> Would bike-shedding the name help, maybe nbdkit_use_default_export,
> to make it obvious that this is instructing nbdkit to reuse
> .default_export?
Yes I think a different name would help here, and the documentation
definitely needs to be updated because I'm very confused by all of
this.

(Also it might have been better if NBD had stuck with "" always
meaning "the default export", but here we are ...)

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/