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

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