On 02/11/2013 05:47 PM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@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