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

[John Ferlan]

On 12/22/2014 04:02 PM, Martin Kletzander wrote:
> 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);

Makes Coverity happy -

ACK

John