[Libguestfs] [nbdkit PATCH 3/6] connections: Add read/write lock over client I/O

[Richard W.M. Jones]

On Sat, Nov 18, 2017 at 05:13:54PM -0600, Eric Blake wrote:
> On 11/18/2017 12:54 PM, Eric Blake wrote:
> > On 11/17/2017 12:28 PM, Eric Blake wrote:
> >>> There's nothing wrong with this patch, but it might be easier to use
> >>> an attribute((cleanup)) handler to deal with the unlocking.  See these
> >>> links for how we do it in libguestfs:
> >>
> >> Oh cool! Yes, that looks nicer.  Although the diffstat for doing so is
> >> larger, because it requires adding to new sub-function {} scopes and
> >> reindenting (we have two different locks in play here).
> >>
> > 
> > Interestingly, it doesn't compile if you lack attribute((cleanup)).
> > Libguestfs has:
> > 
> > common/utils/cleanups.h:/* XXX no safe equivalent to
> > CLEANUP_GL_RECURSIVE_LOCK_UNLOCK */
> > 
> > and will fail to compile if the compiler lacks the attribute - arguably,
> > since no one has reported that compilation failure, it seems no one
> > cares about libguestfs that is not also in possession of modern gcc or
> > clang.  And we already have a configure-time check whether
> > attribute((cleanup)) is detected.  Is it okay to tighten that check to
> > now require a modern compiler for nbdkit, just to get nicer code?
> 
> Another argument in favor of making it mandatory - it is VERY easy to
> write a client that would make nbdkit run out of memory if nbdkit is
> compiled without cleanup support. connections.c uses CLEANUP_FREE char
> *buf, which can be allocated at up to 64M per NBD_CMD_WRITE; all a
> client has to do is send the NBD_CMD_WRITE header, drop the connection,
> and reconnect, and if buf is not auto-freed, nbdkit will leak quite rapidly.
> 
> So that's the approach I'll submit in my next round of patch postings.

Yes it really should be made mandatory (in libguestfs and nbdkit).  We
haven't got around to doing that in libguestfs yet, but if you manage
to find a compiler that doesn't support it, and fixed up the compile
failure, you'd end up with a libguestfs library that would leak memory
like nobody's business.

Rich.

-- 
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