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
--
|: Red Hat, Engineering, London -o-
http://people.redhat.com/berrange/ :|
|:
http://libvirt.org -o-
http://virt-manager.org -o-
http://deltacloud.org :|
|:
http://autobuild.org -o-
http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|