[libvirt] [PATCH v2] Remove qemuDriverLock from almost everywhere

[Martin Kletzander]

On 02/11/2013 05:47 PM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange at redhat.com>
> 
> With the majority of fields in the virQEMUDriverPtr struct
> now immutable or self-locking, there is no need for practically
> any methods to be using the QEMU driver lock. Only a handful
> of helper APIs in qemu_conf.c now need it
> ---
>  src/qemu/THREADS.txt      | 194 +++--------------------
>  src/qemu/qemu_conf.c      |  50 ++++--
>  src/qemu/qemu_domain.c    | 213 +++++--------------------
>  src/qemu/qemu_domain.h    |  40 +----
>  src/qemu/qemu_driver.c    | 386 +++++++++++-----------------------------------
>  src/qemu/qemu_hotplug.c   | 118 +++++++-------
>  src/qemu/qemu_migration.c |  66 ++++----
>  src/qemu/qemu_process.c   | 223 +++++++++-----------------
>  src/qemu/qemu_process.h   |   3 +-
>  9 files changed, 360 insertions(+), 933 deletions(-)
> 
> diff --git a/src/qemu/THREADS.txt b/src/qemu/THREADS.txt
> index c3bad21..785be99 100644
> --- a/src/qemu/THREADS.txt
> +++ b/src/qemu/THREADS.txt
> @@ -14,35 +14,24 @@ Basic locking primitives
>  
>  There are a number of locks on various objects
>  
> -  * struct qemud_driver: RWLock
> +  * virQEMUDriverPtr
>  
> -    This is the top level lock on the entire driver. Every API call in
> -    the QEMU driver is blocked while this is held, though some internal
> -    callbacks may still run asynchronously. This lock must never be held
> -    for anything which sleeps/waits (i.e. monitor commands)
> +    The qemu_conf.h file has inline comments describing the locking
> +    needs for each field. Any field marked immutable, self-locking
> +    can be accessed without the driver lock. For other fields there
> +    are typically helper APIs in qemu_conf.c that provide serialized
> +    access to the data. No code outside qemu_conf.c should ever
> +    acquire this lock
>  

Since this is true, can we make make the locking methods static?  Adding
a syntax-check rule doesn't make sense in this case, I guess.

[...]
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 4917721..ec4c8f8 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -983,9 +938,17 @@ qemuDomainObjAbortAsyncJob(virDomainObjPtr obj)
>      priv->job.asyncAbort = true;
>  }
>  
> +/*
> + * obj must be locked before calling
> + *
> + * To be called immediately before any QEMU monitor API call
> + * Must have already either called qemuDomainObjBeginJob() and checked
> + * that the VM is still active; may not be used for nested async jobs.
> + *
> + * To be followed with qemuDomainObjExitMonitor() once complete
> + */

I'd put this comment before qemuDomainObjEnterMonitor() as that function
is used from outside and you moved it here from
qemuDomainObjEnterMonitorWithDriver() anyway.  But since all the other
things are nit-picks only, I'll send them in a separate patch (prepared
already).  ACK for this one with those qemuDriver{Unlock,Lock} functions
made static.

Martin