[libvirt] [PATCH v2 2/2] qemu: completely rework reference counting

[Michal Privoznik]

On 16.12.2014 11:15, Martin Kletzander wrote:
> There is one problem that causes various errors in the daemon.  When
> domain is waiting for a job, it is unlocked while waiting on the
> condition.  However, if that domain is for example transient and being
> removed in another API (e.g. cancelling incoming migration), it get's
> unref'd.  If the first call, that was waiting, fails to get the job, it
> unref's the domain object, and because it was the last reference, it
> causes clearing of the whole domain object.  However, when finishing the
> call, the domain must be unlocked, but there is no way for the API to
> know whether it was cleaned or not (unless there is some ugly temporary
> variable, but let's scratch that).
>
> The root cause is that our APIs don't ref the objects they are using and
> all use the implicit reference that the object has when it is in the
> domain list.  That reference can be removed when the API is waiting for
> a job.  And because each domain doesn't do its ref'ing, it results in
> the ugly checking of the return value of virObjectUnref() that we have
> everywhere.
>
> This patch changes qemuDomObjFromDomain() to ref the domain (using
> virDomainObjListFindByUUIDRef()) and adds qemuDomObjEndAPI() which
> should be the only function in which the return value of
> virObjectUnref() is checked.  This makes all reference counting
> deterministic and makes the code a bit clearer.
>
> Signed-off-by: Martin Kletzander <mkletzan at redhat.com>
> ---
> v2:
>   - Comments from Peter and Daniel on v1 implemented, rather not
>     listing them here as the list was pretty comprehensive and it would
>     make the reviewer focus on that.
>
>   src/qemu/THREADS.txt      |  40 ++-
>   src/qemu/qemu_domain.c    |  49 ++--
>   src/qemu/qemu_domain.h    |  12 +-
>   src/qemu/qemu_driver.c    | 708 ++++++++++++++++------------------------------
>   src/qemu/qemu_migration.c | 111 +++-----
>   src/qemu/qemu_migration.h |  10 +-
>   src/qemu/qemu_process.c   |  77 ++---
>   7 files changed, 371 insertions(+), 636 deletions(-)
>

> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index f652237..30ea9df 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c

> @@ -4409,8 +4333,7 @@ static void qemuProcessEventHandler(void *data, void *opaque)
>           break;
>       }
>
> -    if (virObjectUnref(vm))
> -        virObjectUnlock(vm);
> +    qemuDomObjEndAPI(&vm);

Interesting, why the heck have we Unref() here? I mean, in the calltrace 
I don't see where the @vm is Ref()-ed. And this should not use the 
reference obtained via qemuMonitorOpen() as we are connecting only once 
and receiving events multiple times. But this is pre-existing code.

>       VIR_FREE(processEvent);
>   }
>

> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index a19e71a..4d9ce09 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c

> @@ -5491,6 +5470,7 @@ int qemuProcessAutoDestroyAdd(virQEMUDriverPtr driver,
>                                 virConnectPtr conn)
>   {
>       VIR_DEBUG("vm=%s, conn=%p", vm->def->name, conn);
> +    virObjectRef(vm);
>       return virCloseCallbacksSet(driver->closeCallbacks, vm, conn,
>                                   qemuProcessAutoDestroy);

This function is called conditionally in qemuProcessStart() depending on 
VIR_QEMU_PROCESS_START_AUTODESTROY flag passed...

>   }
> @@ -5498,9 +5478,12 @@ int qemuProcessAutoDestroyAdd(virQEMUDriverPtr driver,
>   int qemuProcessAutoDestroyRemove(virQEMUDriverPtr driver,
>                                    virDomainObjPtr vm)
>   {
> +    int ret;
>       VIR_DEBUG("vm=%s", vm->def->name);
> -    return virCloseCallbacksUnset(driver->closeCallbacks, vm,
> -                                  qemuProcessAutoDestroy);
> +    ret = virCloseCallbacksUnset(driver->closeCallbacks, vm,
> +                                 qemuProcessAutoDestroy);
> +    virObjectUnref(vm);
> +    return ret;

.. on the other hand, this function is called unconditionally in 
qemuProcessStop(). So after 'virsh start' and 'virsh destroy' the 
reference counts don't match. I've been able to debug this and I came up 
with this diff:

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 4d9ce09..cbd0389 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -5470,7 +5470,6 @@ int qemuProcessAutoDestroyAdd(virQEMUDriverPtr driver,
                                virConnectPtr conn)
  {
      VIR_DEBUG("vm=%s, conn=%p", vm->def->name, conn);
-    virObjectRef(vm);
      return virCloseCallbacksSet(driver->closeCallbacks, vm, conn,
                                  qemuProcessAutoDestroy);
  }
@@ -5482,7 +5481,6 @@ int qemuProcessAutoDestroyRemove(virQEMUDriverPtr 
driver,
      VIR_DEBUG("vm=%s", vm->def->name);
      ret = virCloseCallbacksUnset(driver->closeCallbacks, vm,
                                   qemuProcessAutoDestroy);
-    virObjectUnref(vm);
      return ret;
  }

diff --git a/src/util/virclosecallbacks.c b/src/util/virclosecallbacks.c
index 4f26172..4128057 100644
--- a/src/util/virclosecallbacks.c
+++ b/src/util/virclosecallbacks.c
@@ -138,6 +138,7 @@ virCloseCallbacksSet(virCloseCallbacksPtr 
closeCallbacks,
              VIR_FREE(closeDef);
              goto cleanup;
          }
+        virObjectRef(vm);
      }

      ret = 0;
@@ -172,7 +173,11 @@ virCloseCallbacksUnset(virCloseCallbacksPtr 
closeCallbacks,
          goto cleanup;
      }

-    ret = virHashRemoveEntry(closeCallbacks->list, uuidstr);
+    if (virHashRemoveEntry(closeCallbacks->list, uuidstr) < 0)
+        goto cleanup;
+
+    virObjectUnref(vm);
+    ret = 0;
   cleanup:
      virObjectUnlock(closeCallbacks);
      return ret;


ACK with this (or equivalent) diff squashed in.

Michal