[Libguestfs] [nbdkit PATCH v2 2/8] api: Add nbdkit_add_default_export
Eric Blake
eblake at redhat.com
Thu Aug 27 12:55:48 UTC 2020
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 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);
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
More information about the Libguestfs
mailing list