[Libguestfs] [RFC nbdkit PATCH] protocol: Alter .list_exports, add .default_export

[Richard W.M. Jones]

On Mon, Aug 24, 2020 at 05:02:56PM -0500, Eric Blake wrote:
> On 8/24/20 7:52 AM, Eric Blake wrote:
> >I'm about to add an 'exportname' filter, and in the process, I
> >noticed a few shortcomings in our API.  Time to fix those before
> >the 1.22 release locks our API in stone.  First, .list_exports
> >needs to know if it is pre- or post-TLS, as that may affect which
> >names are exported.  Next, overloading .list_exports to do both
> >NBD_OPT_LIST and mapping "" to a canonical name is proving to be
> >awkward; the canonical mapping is only needed during an
> >NBD_INFO_NAME response to NBD_OPT_GO, and making .open try to
> >grab the entire .list_exports list just to use only the first
> >entry (even if the plugin optimized based on the bool to only
> >provide one entry) is awkward, compared to just having a
> >dedicated function.  Finally, as long as we are going to support
> >NBD_INFO_NAME, we can also support NBD_INFO_DESCRIPTION; but
> >while we map "" to a canonical name prior to calling .open,
> >getting the description makes more sense after the connection
> >is established, alongside .get_size.
> >---
> >
> >I obviously need to finish the code to go with this, but here's where
> >I would like to see the API before we finalize the 1.22 release.
> >
> 
> >+++ b/docs/nbdkit-plugin.pod
> 
> >+=head2 C<.default_export>
> >+
> >+ const char *default_export (int readonly, int is_tls);
> 
> Oh fun.  For some plugins (like ondemand), this is trivial: return a
> compile-time constant string.  But for others (like sh and eval),
> there's a lifetime issue: this callback is used _before_ .open, ergo
> there is no handle struct that it can be associated with.  What's
> more, this is called _after_ .preconnect, which means it is logical
> to expect that the default export name might change over time
> (consider a plugin that advertises the largest file in a directory
> as its default, but where the directory can change _which_ file is
> largest between when the first client connects and when the second
> client connects).  And the string returned by the sh script is in
> malloc'd memory (by it's very nature of coming from the user script,
> rather than being a compile-time constant).  Without a handle to
> store this string in, we would have a memory leak: there is no way
> to associate this inside the handle's struct so that .close can
> reclaim it, but storing it globally is not thread-safe to parallel
> client connections.

I'm not sure there's an actual problem here, because there is still a
connection object inside the server any time we have a TCP connection,
so you can just store it there, unless I'm misunderstanding something.
But anyway ...

> So I'm thinking I need to add a helper
> function:
> 
> const char *nbdkit_string_intern (const char *str);
> 
> Return a pointer to a copy of str, where nbdkit owns the lifetime of
> the copy (allowing the caller to not have to worry about persisting
> str indefinitely).  If called when there is no client connection
> (such as during .load), the copy will remain valid through .unload;
> if called in the context of a client connection (any callback from
> .preconnect through .close), the copy will remain valid through
> .close.

I'm a bit unclear why the plugin would have to call this (or this
function be in the public API at all).  Why can't string interning be
done inside the server.  Have a global struct where strings returned
from the plugin are interned, and free it on server exit.

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/