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

Martin Kletzander mkletzan at redhat.com
Mon Dec 22 21:02:07 UTC 2014


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 at redhat.com>
>> ---
>> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20141222/0ce6008e/attachment-0001.sig>


More information about the libvir-list mailing list