On 02/11/2013 05:47 PM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange@xxxxxxxxxx>
>
> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list