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

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.