[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



On Thu, Jun 17, 2010 at 12:29:34AM +0200, Matthias Bolte wrote:
> 2010/6/16 Daniel P. Berrange <berrange 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.

But every virStoragePool object created, takes a reference on the
virConnectPtr. So if you leak a virStoragePool, you leak a virConnectPtr
too.

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 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]