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

[Martin Kletzander <mkletzan@xxxxxxxxxx>]

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@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?

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);
--

Martin

Attachment: signature.asc
Description: Digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list