Re: [Libguestfs] [RFC nbdkit PATCH] protocol: Alter .list_exports, add .default_export
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