Richard W.M. Jones
2020-Aug-27 07:57 UTC
Re: [Libguestfs] [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
Eric Blake
2020-Aug-27 12:55 UTC
Re: [Libguestfs] [nbdkit PATCH v2 2/8] api: Add nbdkit_add_default_export
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
Richard W.M. Jones
2020-Sep-01 14:19 UTC
Re: [Libguestfs] [nbdkit PATCH v2 2/8] api: Add nbdkit_add_default_export
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/
Seemingly Similar Threads
- Re: [nbdkit PATCH v2 2/8] api: Add nbdkit_add_default_export
- [nbdkit PATCH v2 2/8] api: Add nbdkit_add_default_export
- [nbdkit PATCH 1/5] api: Add .default_export
- [nbdkit PATCH 1/2] python: Implement .list_exports and friends
- [nbdkit PATCH 2/2] ocaml: Implement .list_exports and friends