Re: [libvirt] [PATCH 08/15] Locking of the qemuMonitorPtr object

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 ...