[Libguestfs] [nbdkit PATCH v2 4/8] api: Add nbdkit_str[n]dup_intern helper
Eric Blake
eblake at redhat.com
Tue Sep 1 15:53:01 UTC 2020
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
More information about the Libguestfs
mailing list