[Libguestfs] [nbdkit PATCH v2 2/8] api: Add nbdkit_add_default_export
Richard W.M. Jones
rjones at redhat.com
Thu Aug 27 07:57:35 UTC 2020
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 at 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 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
libguestfs lets you edit virtual machines. Supports shell scripting,
bindings from many languages. http://libguestfs.org
More information about the Libguestfs
mailing list