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. --- daemon/remote.c | 18 +++++++++++++++++- 1 files changed, 17 insertions(+), 1 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index c54565c..1fa0f24 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -979,9 +979,10 @@ remoteDispatchDomainMemoryStats (struct qemud_server *server ATTRIBUTE_UNUSED, /* Allocate stats array for making dispatch call */ if (VIR_ALLOC_N(stats, args->maxStats) < 0) { + virDomainFree (dom); remoteDispatchOOMError(rerr); return -1; - } + } nr_stats = virDomainMemoryStats (dom, stats, args->maxStats, 0); virDomainFree (dom); @@ -1885,6 +1886,7 @@ remoteDispatchDomainMigrateFinish2 (struct qemud_server *server ATTRIBUTE_UNUSED } make_nonnull_domain (&ret->ddom, ddom); + virDomainFree (ddom); return 0; } @@ -5570,10 +5572,12 @@ static int remoteDispatchDomainIsActive(struct qemud_server *server ATTRIBUTE_UN ret->active = virDomainIsActive(domain); if (ret->active < 0) { + virDomainFree(domain); remoteDispatchConnError(err, conn); return -1; } + virDomainFree(domain); return 0; } @@ -5596,10 +5600,12 @@ static int remoteDispatchDomainIsPersistent(struct qemud_server *server ATTRIBUT ret->persistent = virDomainIsPersistent(domain); if (ret->persistent < 0) { + virDomainFree(domain); remoteDispatchConnError(err, conn); return -1; } + virDomainFree(domain); return 0; } @@ -5622,10 +5628,12 @@ static int remoteDispatchInterfaceIsActive(struct qemud_server *server ATTRIBUTE ret->active = virInterfaceIsActive(iface); if (ret->active < 0) { + virInterfaceFree(iface); remoteDispatchConnError(err, conn); return -1; } + virInterfaceFree(iface); return 0; } @@ -5648,10 +5656,12 @@ static int remoteDispatchNetworkIsActive(struct qemud_server *server ATTRIBUTE_U ret->active = virNetworkIsActive(network); if (ret->active < 0) { + virNetworkFree(network); remoteDispatchConnError(err, conn); return -1; } + virNetworkFree(network); return 0; } @@ -5674,10 +5684,12 @@ static int remoteDispatchNetworkIsPersistent(struct qemud_server *server ATTRIBU ret->persistent = virNetworkIsPersistent(network); if (ret->persistent < 0) { + virNetworkFree(network); remoteDispatchConnError(err, conn); return -1; } + virNetworkFree(network); return 0; } @@ -5700,10 +5712,12 @@ static int remoteDispatchStoragePoolIsActive(struct qemud_server *server ATTRIBU ret->active = virStoragePoolIsActive(pool); if (ret->active < 0) { + virStoragePoolFree(pool); remoteDispatchConnError(err, conn); return -1; } + virStoragePoolFree(pool); return 0; } @@ -5726,10 +5740,12 @@ static int remoteDispatchStoragePoolIsPersistent(struct qemud_server *server ATT ret->persistent = virStoragePoolIsPersistent(pool); if (ret->persistent < 0) { + virStoragePoolFree(pool); remoteDispatchConnError(err, conn); return -1; } + virStoragePoolFree(pool); return 0; } -- 1.7.0.4