Libguestfs - Aug 2020 - Re: [RFC nbdkit PATCH] protocol: Alter .list_exports, add .default_export

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/

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

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.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

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