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

On Mon, Dec 22, 2014 at 07:27:29AM -0500, John Ferlan wrote: > > 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? > No, because the vm was gotten using virDomainObjListFindByName() which does not ref it. But instead it should be kept as it was. > >> 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... > Yes, good point. The following diff should suffice for both issues, right? If you agree, do you want me to send a patch or just push it? diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c index 673d8a6..73a825d 100644 --- i/src/qemu/qemu_driver.c +++ w/src/qemu/qemu_driver.c @@ -1516,7 +1516,8 @@ static virDomainPtr qemuDomainLookupByName(virConnectPtr conn, if (dom) dom->id = vm->def->id; cleanup: - virObjectUnlock(vm); + if (vm) + virObjectUnlock(vm); return dom; } diff --git i/src/qemu/qemu_migration.c w/src/qemu/qemu_migration.c index 1db6630..77e0b35 100644 --- i/src/qemu/qemu_migration.c +++ w/src/qemu/qemu_migration.c @@ -3101,7 +3101,9 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, VIR_FREE(xmlout); VIR_FORCE_CLOSE(dataFD[0]); VIR_FORCE_CLOSE(dataFD[1]); - if (ret < 0) { + if (ret < 0 && priv) { + /* priv is set right after vm is added to the list of domains + * and there is no 'goto cleanup;' in the middle of those */ virPortAllocatorRelease(driver->migrationPorts, priv->nbdPort); priv->nbdPort = 0; qemuDomainRemoveInactive(driver, vm);