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@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);
Makes Coverity happy -
ACK
John
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list