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 <[email protected]>
---
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
_______________________________________________
Libguestfs mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/libguestfs
