Re: [Qemu-devel] [RFC PATCH 0/5] Scoped locks using attribute((cleanup))

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2936 bytes --]

On 12/08/2017 04:55 AM, Paolo Bonzini wrote:
> This is an attempt to make a C API that resembles the C++
> std::unique_lock (mostly untested).  The idea is that you can write
> 
>     QEMU_LOCK_GUARD(QemuMutex, guard_name, &some_mutex);
> 
> instead of
> 
>     qemu_mutex_lock(&some_mutex);
>     ...
> out:
>     qemu_mutex_unlock(&some_mutex);
> 
> and the mutex will be unlocked on all exit paths.  In C++ that
> would be "std::unique_lock<QemuMutex> guard_name(some_mutex);".
> Likewise,
> 
>     QEMU_WITH_LOCK(QemuMutex, guard_name, &some_mutex) {
>         ...
>     }
> 
> is the same as
> 
>     qemu_mutex_lock(&some_mutex);
>     ...
>     qemu_mutex_unlock(&some_mutex);
> 
> except that any returns within the region will unlock the mutex.

Not just returns, but ANY manner that you leave the scope, including a
goto or just falling out of the end of the scope.

(and, as Stefan pointed out, this poisons the use of 'break' when this
is used inside a loop, as it now breaks the scope of the QEMU_WITH_LOCK
instead of the outer loop).

> It's possible to use QemuLockGuard also with a spinlock or a
> CoMutex.  However, it is _not_ possible to return a QemuLockGuard
> since C doesn't have an equivalent of C++'s "move semantics", so
> there are other "constructor" macros such as QEMU_ADOPT_LOCK
> and QEMU_TAKEN_LOCK.
> 
> On the positive side, I checked that the entire abstraction
> is removed by the compiler, the generated code is more or less
> the same.  Also, it would let us for example change block/curl.c
> to use a CoQueue instead of a home-grown list+QemuMutex.
> 
> However, I am still not sure about the readability, and it doesn't play
> well with another common idiom in QEMU, which is to wrap global mutexes
> with function such as
> 
>     static void block_job_lock(void)
>     {
>         qemu_mutex_lock(&block_job_mutex);
>     }
> 
>     static void block_job_unlock(void)
>     {
>         qemu_mutex_unlock(&block_job_mutex);
>     }
> 
> So I'm a bit underwhelmed by this experiment.  Other opinions?

The semantics you list here:

>     QEMU_WITH_LOCK(QemuMutex, guard_name, &some_mutex) {
>         ...
>     }

are slightly different from the semantics in nbdkit:

{
  ACQUIRE_LOCK_FOR_CURRENT_SCOPE(&some_lock);
  ...
}

In that your version requires the user to supply the scope after your
macro, instead of using your macro within the scope.  But I guess that's
because you added other helpers like QEMU_WITH_ADOPTED_LOCK,
QEMU_RETURN_LOCK, and QEMU_TAKEN_LOCK, where you have multiple
possibilities for which state the lock should be in.

Is it worth playing around with the user providing the scope around your
macros, instead of using your macro to introduce the scope?

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]