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@xxxxxxxxxx>
> ---
> 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...
> + priv->nbdPort = 0;
> + qemuDomainRemoveInactive(driver, vm);
> }
> + qemuDomObjEndAPI(&vm);
> if (event)
> qemuDomainEventQueue(driver, event);
> qemuMigrationCookieFree(mig);
> @@ -3137,8 +3118,7 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver,
> qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, 0);
>
> endjob:
> - if (!qemuMigrationJobFinish(driver, vm))
> - vm = NULL;
> + qemuMigrationJobFinish(driver, vm);
> goto cleanup;
> }
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list