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

My Coverity scan found two issues both FORWARD_NULL... qemuDomainLookupByName and qemuMigrationPrepareAny On 12/16/2014 05:15 AM, 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(-) > <...snip...> > @@ -1517,8 +1515,7 @@ static virDomainPtr qemuDomainLookupByName(virConnectPtr conn, > if (dom) dom->id = vm->def->id; > > cleanup: > - if (vm) > - virObjectUnlock(vm); > + virObjectUnlock(vm); Um... Did you mean the qemuDomObjEndAPI call here?

> return dom; > } > <...snip...> > @@ -3099,12 +3089,7 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, > * This prevents any other APIs being invoked while incoming > * migration is taking place. > */ > - if (!qemuMigrationJobContinue(vm)) { > - vm = NULL; > - virReportError(VIR_ERR_OPERATION_FAILED, > - "%s", _("domain disappeared")); > - goto cleanup; > - } > + qemuMigrationJobContinue(vm); > > if (autoPort) > priv->migrationPort = port; > @@ -3115,16 +3100,12 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, > VIR_FREE(xmlout); > VIR_FORCE_CLOSE(dataFD[0]); > VIR_FORCE_CLOSE(dataFD[1]); > - if (vm) { > - if (ret < 0) { > - virPortAllocatorRelease(driver->migrationPorts, priv->nbdPort); > - priv->nbdPort = 0; > - } > - if (ret >= 0 || vm->persistent) > - virObjectUnlock(vm); > - else > - qemuDomainRemoveInactive(driver, vm); > + if (ret < 0) { > + virPortAllocatorRelease(driver->migrationPorts, priv->nbdPort); We can get here with priv == NULL from numerous places...