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

[Eric Blake]

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

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