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

[Daniel Veillard]

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 at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/