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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]