[Libguestfs] [nbdkit PATCH v2 10/17] plugins: Add .fork_safe field

[Eric Blake]

On 8/2/19 2:52 PM, Richard W.M. Jones wrote:
> On Fri, Aug 02, 2019 at 02:26:11PM -0500, Eric Blake wrote:
>> Allow a plugin field to declare whether a parallel plugin can tolerate
>> windows where fds are not CLOEXEC, or must take precautions to avoid
>> leaking fds if the plugin may fork.  For safety reasons, the flag
>> defaults to off, but many in-tree plugins can set it to on (most
>> commonly because they don't fork after .config_complete; for libvirt
>> because it is documented to clean up fds on fork so it is immune to
>> anything we might leak; for libnbd because we don't use the API that
>> forks).  Note that I did not choose to set the new field for many of
>> the various language bindings (it becomes a rather difficult task to
>> prove whether the third-party language binding code is itself using
>> atomic CLOEXEC or fd sanitization).  However many of our languages are
>> still stuck as serialized, and the lack of .fork_safe won't impact
>> those thread model anyways.
>>
>> Update the testsuite to skip parallel tests that would otherwise fail
>> when the thread model is crippled.
>>
>> Upcoming patches will then fix the server to audit and fix places
>> where we currently leak fds, and then cripple the thread model only on
>> platforms where atomic CLOEXEC is not possible.
> 
> My worry about this patch is we're adding a new plugin flag which
> we'll have to support forever, but IIUC it's only needed on one
> platform (ie. Haiku) which really ought to get fixed.  In future we'll
> end up in a situation where we have this flag but it's no longer
> needed.

Yeah, the fact that it is a no-op on Linux means it's hard to test.

Still, the idea of penalizing all plugins, even though only the forking
plugins care, is a bit hard to swallow - maybe that will prod Haiku in
catching up faster.

> How about instead of this we simply restrict Haiku to the fully
> serialized mode.  Sucks for them, but they can fix it by adding atomic
> CLOEXEC features ...

Makes sense. I'll still want to ensure that --dump-plugin shows the
dynamic crippling (so that we can easily skip
tests/test-parallel-file.sh, for example), but I think I can manage to
do that by replacing this patch with just blind penalization of
non-atomic CLOEXEC.

> 
>> +  .fork_safe         = 0, /* libguestfs uses fork(), unaudited for safety */
> 
> libguestfs does use fork and should be safe - we're pretty careful
> about using CLOEXEC, accept4, etc everywhere.  (Also libguestfs
> doesn't run on Haiku and architecturally that's unlikely to ever
> happen).

Nice to know. That was half of the review - determining which libraries
use which functions, and what might still need fixing (or bugs filed to
other projects).  It also looks like libnbd is using CLOEXEC everywhere
that it should.  But as long as the fault is no longer nbdkit's, any
further bugs we get about fd leaks can be redirected to whoever is
actually doing the leak.

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20190802/d923702a/attachment.sig>