On Wed, Nov 04, 2009 at 05:51:36PM +0100, Daniel Veillard wrote:
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 ...
Yes, I think I'll do a further add-on patch to make that change. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|