Lock guards automatically call qemu_(rec_)mutex_unlock() when returning from a
function or leaving leaving a lexical scope.  This simplifies code and
eliminates leaks (especially in error code paths).

This series adds lock guards for QemuMutex and QemuRecMutex.  It does not
convert the entire tree but includes example conversions.

Stefan Hajnoczi (2):
  thread: add QemuRecMutex lock guards
  thread: add QemuMutex lock guards

 include/qemu/thread.h | 52 +++++++++++++++++++++++++++++++++++++++++++
 plugins/core.c        |  6 ++---
 plugins/loader.c      | 15 ++++++-------
 util/qemu-timer.c     | 22 +++++++++---------
 4 files changed, 71 insertions(+), 24 deletions(-)

-- 
2.24.1

Patchew URL: https://patchew.org/QEMU/20200311123624.277221-1-stefanha@redhat.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC      util/hexdump.o
  CC      util/crc32c.o
/tmp/qemu-test/src/util/qemu-timer.c: In function 'timerlist_expired':
/tmp/qemu-test/src/util/qemu-timer.c:196:5: error: 'expire_time' may be used uninitialized in this function [-Werror=maybe-uninitialized]
     return expire_time <= qemu_clock_get_ns(timer_list->clock->type);
     ^
cc1: all warnings being treated as errors
  CC      util/uuid.o
make: *** [util/qemu-timer.o] Error 1
make: *** Waiting for unfinished jobs....
rm tests/qemu-iotests/socket_scm_helper.o
Traceback (most recent call last):
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=92163d9ae7a944769ca832267d70514c', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-ijn0d86j/src/docker-src.2020-03-11-09.18.18.28503:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=92163d9ae7a944769ca832267d70514c
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-ijn0d86j/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    1m59.199s
user    0m7.765s


The full log is available at
http://patchew.org/logs/20200311123624.277221-1-stefanha@redhat.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Stefan Hajnoczi <stefanha@redhat.com> writes:

> Lock guards automatically call qemu_(rec_)mutex_unlock() when returning from a
> function or leaving leaving a lexical scope.  This simplifies code and
> eliminates leaks (especially in error code paths).
>
> This series adds lock guards for QemuMutex and QemuRecMutex.  It does not
> convert the entire tree but includes example conversions.

I support the move towards automatic cleanup, but I'm wary of
incremental conversion.  Experience tells that outdated examples
invariably get copied / imitated, with incremental conversion struggling
to keep up.

Are we resigned to having both kinds of mutex cleanup forever?

If not, what's the plan to get the job finished, and until when?

Il mer 11 mar 2020, 15:50 Markus Armbruster <armbru@redhat.com> ha scritto:

> Stefan Hajnoczi <stefanha@redhat.com> writes:
>
> > Lock guards automatically call qemu_(rec_)mutex_unlock() when returning
> from a
> > function or leaving leaving a lexical scope.  This simplifies code and
> > eliminates leaks (especially in error code paths).
> >
> > This series adds lock guards for QemuMutex and QemuRecMutex.  It does not
> > convert the entire tree but includes example conversions.
>
> I support the move towards automatic cleanup, but I'm wary of
> incremental conversion.  Experience tells that outdated examples
> invariably get copied / imitated, with incremental conversion struggling
> to keep up.
>
> Are we resigned to having both kinds of mutex cleanup forever?
>

There are cases where the legibility benefits of guards are debatable, or
they require more complex functionality in the guards (see my other answer
to Stefan). So, yes. We don't have that many mutexes so incremental
conversion should be doable without taking forever.

Paolo


> If not, what's the plan to get the job finished, and until when?
>
>

On Wed, Mar 11, 2020 at 04:06:02PM +0100, Paolo Bonzini wrote:
> Il mer 11 mar 2020, 15:50 Markus Armbruster <armbru@redhat.com> ha scritto:
> 
> > Stefan Hajnoczi <stefanha@redhat.com> writes:
> >
> > > Lock guards automatically call qemu_(rec_)mutex_unlock() when returning
> > from a
> > > function or leaving leaving a lexical scope.  This simplifies code and
> > > eliminates leaks (especially in error code paths).
> > >
> > > This series adds lock guards for QemuMutex and QemuRecMutex.  It does not
> > > convert the entire tree but includes example conversions.
> >
> > I support the move towards automatic cleanup, but I'm wary of
> > incremental conversion.  Experience tells that outdated examples
> > invariably get copied / imitated, with incremental conversion struggling
> > to keep up.
> >
> > Are we resigned to having both kinds of mutex cleanup forever?
> >
> 
> There are cases where the legibility benefits of guards are debatable, or
> they require more complex functionality in the guards (see my other answer
> to Stefan). So, yes. We don't have that many mutexes so incremental
> conversion should be doable without taking forever.

I will add this to the BiteSizedTasks wiki page when the patch is
merged, together with guidelines on how to convert code (it requires
case-by-case evaluation and is not a simple mechanical change).

We will continue to have raw qemu_(rec_)mutex_lock/unlock() calls in
cases where a complex locking scheme is used or lock guards would make
the code less clear.

Stefan

Patchew URL: https://patchew.org/QEMU/20200311123624.277221-1-stefanha@redhat.com/



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#! /bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-mingw@fedora J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC      util/uri.o
  CC      util/notify.o
/tmp/qemu-test/src/util/qemu-timer.c: In function 'timerlist_expired':
/tmp/qemu-test/src/util/qemu-timer.c:196:24: error: 'expire_time' may be used uninitialized in this function [-Werror=maybe-uninitialized]
     return expire_time <= qemu_clock_get_ns(timer_list->clock->type);
            ~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
make: *** [/tmp/qemu-test/src/rules.mak:69: util/qemu-timer.o] Error 1
make: *** Waiting for unfinished jobs....
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 664, in <module>
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=bbe7ec3619e2401a97060d5d9ab55bac', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-jdtnwksa/src/docker-src.2020-03-11-09.20.54.2854:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-mingw']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=bbe7ec3619e2401a97060d5d9ab55bac
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-jdtnwksa/src'
make: *** [docker-run-test-mingw@fedora] Error 2

real    1m36.654s
user    0m7.856s


The full log is available at
http://patchew.org/logs/20200311123624.277221-1-stefanha@redhat.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Il mer 11 mar 2020, 13:38 Stefan Hajnoczi <stefanha@redhat.com> ha scritto:

> Lock guards automatically call qemu_(rec_)mutex_unlock() when returning
> from a
> function or leaving leaving a lexical scope.  This simplifies code and
> eliminates leaks (especially in error code paths).
>
> This series adds lock guards for QemuMutex and QemuRecMutex.  It does not
> convert the entire tree but includes example conversions.
>

Thanks for picking this up! It should be possible to use QemuLockable to
introduce a single set of lock guard macros that work for mutexes,
spinlocks and CoMutexes. Would you look into that?

(C++ also has unique_lock, a kind of lock guard that can be unlocked early
and won't cause a double unlock, and also can be created unlocked. However
it makes sense to not implement that unless one has a killer application of
it in the tree).

Paolo


> Stefan Hajnoczi (2):
>   thread: add QemuRecMutex lock guards
>   thread: add QemuMutex lock guards
>
>  include/qemu/thread.h | 52 +++++++++++++++++++++++++++++++++++++++++++
>  plugins/core.c        |  6 ++---
>  plugins/loader.c      | 15 ++++++-------
>  util/qemu-timer.c     | 22 +++++++++---------
>  4 files changed, 71 insertions(+), 24 deletions(-)
>
> --
> 2.24.1
>
>

On Wed, Mar 11, 2020 at 01:52:35PM +0100, Paolo Bonzini wrote:
> Il mer 11 mar 2020, 13:38 Stefan Hajnoczi <stefanha@redhat.com> ha scritto:
> 
> > Lock guards automatically call qemu_(rec_)mutex_unlock() when returning
> > from a
> > function or leaving leaving a lexical scope.  This simplifies code and
> > eliminates leaks (especially in error code paths).
> >
> > This series adds lock guards for QemuMutex and QemuRecMutex.  It does not
> > convert the entire tree but includes example conversions.
> >
> 
> Thanks for picking this up! It should be possible to use QemuLockable to
> introduce a single set of lock guard macros that work for mutexes,
> spinlocks and CoMutexes. Would you look into that?
> 
> (C++ also has unique_lock, a kind of lock guard that can be unlocked early
> and won't cause a double unlock, and also can be created unlocked. However
> it makes sense to not implement that unless one has a killer application of
> it in the tree).

I already looked at lockable.h and to be honest I didn't want to combine
g_autoptr macros with lockable's polymorphism macros.

However, since you have mentioned it I'll take another look and try to
overcome the aversion :).

Stefan