[libvirt] [PATCH] Add several missing vir*Free calls in libvirtd's remote code

[Matthias Bolte]

2010/6/16 Daniel P. Berrange <berrange at redhat.com>:
> On Sat, Jun 12, 2010 at 06:32:55PM +0200, Matthias Bolte wrote:
>> Justin Clift reported a problem with adding virStoragePoolIsPersistent
>> to virsh's pool-info command, resulting in a strange problem. Here's
>> an example:
>>
>>     virsh # pool-create-as images_dir3 dir - - - - "/home/images2"
>>     Pool images_dir3 created
>>
>>     virsh # pool-info images_dir3
>>     Name:           images_dir3
>>     UUID:           90301885-94eb-4ca7-14c2-f30b25a29a36
>>     State:          running
>>     Capacity:       395.20 GB
>>     Allocation:     30.88 GB
>>     Available:      364.33 GB
>>
>>     virsh # pool-destroy images_dir3
>>     Pool images_dir3 destroyed
>>
>> At this point the images_dir3 pool should be gone (because it was
>> transient) and we should be able to create a new pool with the same name:
>>
>>     virsh # pool-create-as images_dir3 dir - - - - "/home/images2"
>>     Pool images_dir3 created
>>
>>     virsh # pool-info images_dir3
>>     Name:           images_dir3
>>     UUID:           90301885-94eb-4ca7-14c2-f30b25a29a36
>>     error: Storage pool not found
>>
>> The new pool got the same UUID as the first one, but we didn't specify
>> one. libvirt should have picked a random UUID, but it didn't.
>>
>> It turned out that virStoragePoolIsPersistent leaks a reference to the
>> storage pool object (actually remoteDispatchStoragePoolIsPersistent does).
>> As a result, pool-destroy doesn't remove the virStoragePool for the
>> "images_dir3" pool from the virConnectPtr's storagePools hash on libvirtd's
>> side. Then the second pool-create-as get's the stale virStoragePool object
>> associated with the "images_dir3" name. But this object has the old UUID.
>>
>> This commit ensures that all get_nonnull_* and make_nonnull_* calls for
>> libvirt objects are matched properly with vir*Free calls. This fixes the
>> reference leaks and the reported problem.
>>
>> All remoteDispatch*IsActive and remoteDispatch*IsPersistent functions were
>> affected. But also remoteDispatchDomainMigrateFinish2 was affected in the
>> success path. I wonder why that didn't surface earlier. Probably because
>> domainMigrateFinish2 is executed on the destination host and in the common
>> case this connection is opened especially for the migration and gets closed
>> after the migration is done. So there was no chance to run into a problem
>> because of the leaked reference.
>
> ACK, looks good.
>
> Wonder why coverity hadn't flagged these memory leaks before..
>
> Daniel
>

Probably because the reference leak doesn't result in a memory leak.
The old virStoragePool objects in the virConnectPtr's storagePools
hash are freed in virReleaseConnect.

Thanks, pushed.

Matthias