[libvirt] [PATCH 2/5] qemu: minor monitor lock cleanups

[Michal Privoznik]

On 23.02.2013 00:09, Eric Blake wrote:
> If virCondInit fails (okay, so that's unlikely), then we end up
> attempting a virObjectUnlock() on the cleanup path, even though
> we don't hold a lock.  This is not guaranteed to be safe.  While
> at it, I noticed a couple places where we were referencing mon->fd
> outside locks.
> 
> * src/qemu/qemu_monitor.c (qemuMonitorOpenInternal): Minimize lock
> duration.  mon-watch doesn't need clean up on error.
> (qemuMonitorGetBlockExtent, qemuMonitorBlockResize): Don't
> dereference fd outside of lock.
> ---
>  src/qemu/qemu_monitor.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 7f4a7a0..40eea75 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -717,7 +717,6 @@ qemuMonitorOpenInternal(virDomainObjPtr vm,
>      if (json)
>          mon->wait_greeting = 1;
>      mon->cb = cb;
> -    virObjectLock(mon);
> 
>      if (virSetCloseExec(mon->fd) < 0) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -731,6 +730,7 @@ qemuMonitorOpenInternal(virDomainObjPtr vm,
>      }
> 
> 
> +    virObjectLock(mon);
>      virObjectRef(mon);
>      if ((mon->watch = virEventAddHandle(mon->fd,
>                                          VIR_EVENT_HANDLE_HANGUP |
> @@ -740,6 +740,7 @@ qemuMonitorOpenInternal(virDomainObjPtr vm,
>                                          mon,
>                                          virObjectFreeCallback)) < 0) {
>          virObjectUnref(mon);
> +        virObjectUnlock(mon);

Heh, it took me while to realize this is good actually. I thought it
should be vice versa to prevent Unlock() being called on just freed
memory. But then I realized after the Unref() mon refcount == 1.

>          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                         _("unable to register monitor events"));
>          goto cleanup;
> @@ -759,11 +760,8 @@ cleanup:
>       * so kill the callbacks now.
>       */
>      mon->cb = NULL;
> -    virObjectUnlock(mon);
>      /* The caller owns 'fd' on failure */
>      mon->fd = -1;
> -    if (mon->watch)
> -        virEventRemoveHandle(mon->watch);
>      qemuMonitorClose(mon);
>      return NULL;
>  }
> @@ -1508,8 +1506,7 @@ int qemuMonitorGetBlockExtent(qemuMonitorPtr mon,
>                                unsigned long long *extent)
>  {
>      int ret;
> -    VIR_DEBUG("mon=%p, fd=%d, dev_name=%p",
> -          mon, mon->fd, dev_name);
> +    VIR_DEBUG("mon=%p, dev_name=%p", mon, dev_name);
> 
>      if (mon->json)
>          ret = qemuMonitorJSONGetBlockExtent(mon, dev_name, extent);
> @@ -1524,8 +1521,7 @@ int qemuMonitorBlockResize(qemuMonitorPtr mon,
>                             unsigned long long size)
>  {
>      int ret;
> -    VIR_DEBUG("mon=%p, fd=%d, devname=%p size=%llu",
> -              mon, mon->fd, device, size);
> +    VIR_DEBUG("mon=%p, devname=%p size=%llu", mon, device, size);
> 
>      if (mon->json)
>          ret = qemuMonitorJSONBlockResize(mon, device, size);
> 

ACK

Michal