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

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?

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

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.

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

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