[Libguestfs] [nbdkit PATCH v2 4/8] api: Add nbdkit_str[n]dup_intern helper

[Eric Blake]

On 9/1/20 9:24 AM, Richard W.M. Jones wrote:
> On Wed, Aug 26, 2020 at 09:16:48PM -0500, Eric Blake wrote:
>> Implementing .default_export with its 'const char *' return is tricky
>> in the sh plugin: we must return dynamic memory, but must avoid a
>> use-after-free.  And we don't want to change the return type of
>> .default_export to 'char *', because that would make our choice of
>> malloc()/free() part of the API, preventing either nbdkit or a plugin
>> from experimenting with an alternate allocator implementation.
>> Elsewhere, we have done things like nbdkit_extents_new(), so that even
>> though the client is directing allocation, the actual call to malloc()
>> is done by nbdkit proper, so that nbdkit later calling free() does not
>> tie the plugin's hands, and nbdkit could change its underlying
>> allocation without breaking ABI.  (Note that nbdkit_realpath() and
>> nbdkit_absolute_path() require the caller to use free(), but as they
>> are used during .config, it's less of a burden for plugins to take
>> care of that in .unload.)
>>
>> We could document that the burden is on the plugin to avoid the memory
>> leak, by making sh track all strings it returns, then finally clean
>> them up during .unload.  But this is still a memory leak in the case
>> of .default_export, which can be called multiple times when there are
>> multiple clients, and presumably with different values per client
>> call; better would be freeing the memory at .close when each client
>> goes away.  Except that .default_export is called before .open, and
>> therefore before we have access to the handle struct that will
>> eventually make it to .close.
>>
>> So, the solution is to let nbdkit track an alternate string on our
>> behalf, where nbdkit then cleans it up as the client goes away.
>>
>> There are several places in the existing code base that can also take
>> advantage of this copying functionality, including in filters that
>> want to pass altered strings to .config while still obeying lifetime
>> rules.
> 
> We should probably just push this one straight away, with
> one small change:
> 
>> diff --git a/server/public.c b/server/public.c
>> index d10d466e..512e9caa 100644
>> --- a/server/public.c
>> +++ b/server/public.c
> 
> This file is probably going to need:
> 
>    #include "strndup.h"
> 
> somewhere near the top because unbelievably Win32 lacks this function.

Sounds good.  I'm also finding that nbdkit_printf_intern(const char 
*fmt, ...) and nbdkit_vprintf_intern(const char *, va_list) are going to 
be useful, so I'll add those, get this in, then post a v3 of the rest of 
the series with further doc cleanups.

Another use for this new function (capturing the essence of an IRC 
discussion): we have several language plugins that mention that things 
like .config_help is not overrideable.  The full set of struct plugin 
strings that would be nicer as functions:
.longname
.description
.config_help
.magic_config_key

(Maybe .name or .version as well, but I see less reason to want to make 
those dynamic)

Having a callback function that returns a const char * would let 
language bindings return a description or similar as set by the script; 
it would also allow us to alter the magic config key in a stateful 
manner according to what previous config keys have been parsed.  So, for 
example, we could allow:

nbdkit python file.py myfile

as shorthand for

nbdkit python script=file.py file=myfile

by having file.py provide an overload for a new .get_magic_config_key() 
function.

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