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

[Richard W.M. Jones]

On Tue, Aug 25, 2020 at 06:16:17AM -0500, Eric Blake wrote:
> On 8/25/20 5:00 AM, Richard W.M. Jones wrote:
> 
> >>>+=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.
> 
> Plugins cannot access the TCP connection except through public APIs ;)

Sure, plugins can't, but there's some code in the server which you've
written which could be doing:

  GET_CONN;
  const char *new_name;
  ...
  new_name = plugin->default_export (...);
  if (conn->default_exportname != NULL)
    free (conn->default_exportname);
  conn->default_exportname = strdup (new_name);

> I've got the patch working (it passes 'make check-valgrind'), so
> I'll post it later this morning to demo why it is useful.
> 
> >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.
> 
> The server IS doing the interning, and associating the intern'd
> strings with the connection or with overall server exit.  But
> plugins need to use it when they need a longer lifetime but don't
> want to clean up themselves.

I must be missing something here ...

Rich.

-- 
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