On Tue, Nov 03, 2009 at 02:50:02PM -0500, Daniel P. Berrange wrote:
In preparation of the monitor I/O process becoming fully asynchronous, it is neccessary to ensure all access to internals of the qemuMonitorPtr object is protected by a mutex lock.
* src/qemu/qemu_monitor.c, src/qemu/qemu_monitor.h: Add mutex for locking monitor. * src/qemu/qemu_driver.c: Add locking around all monitor commands --- src/qemu/qemu_driver.c | 299 +++++++++++++++++++++++++++++++++++------------ src/qemu/qemu_monitor.c | 19 +++ src/qemu/qemu_monitor.h | 3 + 3 files changed, 248 insertions(+), 73 deletions(-)
[..]
+++ b/src/qemu/qemu_driver.c @@ -141,6 +141,22 @@ static void qemuDomainObjPrivateFree(void *data) }
+static void qemuDomainObjEnterMonitor(virDomainObjPtr obj) +{ + qemuDomainObjPrivatePtr priv = obj->privateData; + + qemuMonitorLock(priv->mon); +} + + +static void qemuDomainObjExitMonitor(virDomainObjPtr obj) +{ + qemuDomainObjPrivatePtr priv = obj->privateData; + + qemuMonitorUnlock(priv->mon); +} +
so we're using pthread mutex here and basically there is no way this could go wrong and there is no need to handle errors, okay
static int qemuCgroupControllerActive(struct qemud_driver *driver, int controller) {
the patch is quite regular
+ qemuDomainObjEnterMonitor(vm); + if (qemuMonitorStopCPUs(priv->mon) < 0) { + qemuDomainObjExitMonitor(vm); goto cleanup; + } + qemuDomainObjExitMonitor(vm);
to some extend I wonder if we shouldn't kill all those blocks and always use the more regular form qemuDomainObjEnterMonitor(vm); ret = qemuMonitorStopCPUs(priv->mon); qemuDomainObjExitMonitor(vm); if (ret < 0) goto cleanup; it's simpler, easier to read, we don't have to think about matching of Enter and Exit, and the generated code probably will be identical In a number of places the refactoring introduced the later way but the former is still present in a number of place, closer to original code but doesn't make the patch safer I'm afraid. The only inconvenience is the declaration of the temporary variable ... [...]
+void qemuMonitorLock(qemuMonitorPtr mon) +{ + virMutexLock(&mon->lock); +} + +void qemuMonitorUnlock(qemuMonitorPtr mon) +{ + virMutexUnlock(&mon->lock); +}
okay no error possible ... Okay, I didn't spot anything suspect in the locking patch. But as usual the problems may be in parts we may have forgotten about. Also sometime the patch shows only a fraction of the relevant code. Best is to push and test, ACK ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/