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

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

On 8/25/20 7:21 AM, Richard W.M. Jones wrote:
> 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);
So you're suggesting that the signature for .default_export should 
return 'char *' of malloc()d storage, and that nbdkit is then 
responsible for free()ing that string.  It's an ABI coupling; it means 
we are forcing the plugin and server to agree on which allocation 
routines to use for any information passed across that boundary.  And it 
forces plugins to malloc memory, even when their answer is a 
compile-time constant.

True, we've already done that with nbdkit_realpath and 
nbdkit_absolute_path (nbdkit calls malloc(), so the plugin must call 
free()), but elsewhere we've tried avoiding it to reduce the ABI 
dependency and allow us (either the server or the plugin) the freedom to 
use an alternative allocator scheme (witness nbdkit_extents_new() - the 
client needs memory allocated, but that allocation is wrapped inside a 
public nbdkit function, so nbdkit itself is now responsible for both the 
allocation and freeing of the memory, and is no longer limited to 
malloc()/free() dependency).
>> 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 ...
I'd better post the patch, then, as code speaks better than prose.

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

I think I get where my misunderstanding is now.  I thought the problem
was that the server could not store the string, but your concern is
that the plugin cannot free the string after returning it to the
caller (server).

I can't really think of a better way to do it than the interning
option you presented in the initial email.  Interning might also be
useful in future cases where we want to send strings to the server
from the plugin.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top