[Libguestfs] [nbdkit PATCH 8/8] rate: Atomically set CLOEXEC on fds

[Richard W.M. Jones]

On Thu, Aug 01, 2019 at 05:09:58AM -0500, Eric Blake wrote:
> On 8/1/19 4:12 AM, Richard W.M. Jones wrote:
> > On Wed, Jul 31, 2019 at 05:01:52PM -0500, Eric Blake wrote:
> >> On 7/31/19 4:31 PM, Eric Blake wrote:
> >>> The rate filter is potentially opening fds in one thread while another
> >>> thread is processing a fork() in the plugin.  Although the file is not
> >>> open for long, we MUST atomically use CLOEXEC to avoid fd leaks.  This
> >>> one is a bit harder to observe using only the sh plugin, because the
> >>> window is small; you'll have better success at catching the leak by
> >>> using gdb or recompiling code to insert strategic sleeps.
> >>
> >> In fact, I have to tweak this commit message: you CAN'T observe this one
> >> with the sh plugin unless you recompile it to use #define THREAD_MODEL
> >> NBDKIT_THREAD_MODEL_SERIALIZE_REQUESTS, as well as introducing the
> >> timing hacks mentioned above (that's because with our current
> >> SERIALIZE_ALL_REQUESTS, there is never more than one thread in
> >> filter/plugin code at a time).
> > 
> > The current nbdkit-sh-plugin is only SERIALIZE_ALL_REQUESTS in order
> > to make writing the shell scripts a bit more sane.  I believe it could
> > be fully PARALLEL.
> 
> Other than the fact that it uses pipe() instead of pipe2(), I'm not
> seeing any other strong reasons why it can't be parallel.  I'll change
> patch 9 along those lines.
> 
> > 
> > (As an aside: Ideally in future we'll allow the thread model to be
> > specified by the plugin dynamically.  It's one of the things I thought
> > I had listed in the TODO file - it wasn't there so I've added it now.)
> 
> That's because we already have that: See commit afbcd070 and nearby.  So
> I'll just revert your TODO change :)

Indeed we do.  Should add that to all the language plugins at some
point ...

Rich.

> >> But it does raise an interesting point - if we hit platforms that are
> >> unable to support atomic CLOEXEC, one possibility is a patch that forces
> >> SERIALIZE_ALL_REQUESTS as the maximum parallelism allowed on that
> >> platform (while remaining at our goal of PARALLEL on more competent
> >> systems) - once we do that, the lacking systems will be serialized to
> >> the point that there is no race window where one thread can fork() while
> >> another is obtaining an fd.
> > 
> > Yup.  But probably better to encourage those platforms to support
> > atomic CLOEXEC everywhere.
> 
> Yes, it would be nice for Haiku to realize how much they are losing out
> on by not providing it.
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3226
> Virtualization:  qemu.org | libvirt.org
> 




-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html