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

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(a)redhat.com&gt; > --- > 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