Re: [libvirt] [PATCH] Fix destroy command memory leaks
Cole Robinson wrote:
Some pieces of libvirt currently assume that the vir*Destroy
functions will free the passed object upon success. In
practice none of the current drivers seem to do this,
resulting in memory leaks.
The attached patch fixes the leaks I could find, as well as
changes the comments for virDomainDestroy and virNetworkDestroy
in libvirt.c to reflect reality. I also added a couple debug
statements to hash.c where domain reference counts can be
printed as they are changed.
Small update to the patch fixing the debug messages in hash.c
to be less redundant and more useful :)
Thanks,
Cole
diff --git a/qemud/remote.c b/qemud/remote.c
index a97641a..725152e 100644
--- a/qemud/remote.c
+++ b/qemud/remote.c
@@ -940,9 +940,11 @@ remoteDispatchDomainDestroy (struct qemud_server *server
ATTRIBUTE_UNUSED,
return -2;
}
- if (virDomainDestroy (dom) == -1)
+ if (virDomainDestroy (dom) == -1) {
+ virDomainFree(dom);
return -1;
- /* No need to free dom - destroy does it for us */
+ }
+ virDomainFree(dom);
return 0;
}
diff --git a/src/hash.c b/src/hash.c
index 79421aa..a014990 100644
--- a/src/hash.c
+++ b/src/hash.c
@@ -842,6 +842,9 @@ __virGetDomain(virConnectPtr conn, const char *name, const unsigned
char *uuid)
goto error;
}
conn->refs++;
+ DEBUG("New hash entry %p", ret);
+ } else {
+ DEBUG("Existing hash entry %p: refs now %d", ret, ret->refs+1);
}
ret->refs++;
pthread_mutex_unlock(&conn->lock);
diff --git a/src/libvirt.c b/src/libvirt.c
index 97f6bc3..9f6df8e 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -1390,10 +1390,9 @@ virDomainLookupByName(virConnectPtr conn, const char *name)
* @domain: a domain object
*
* Destroy the domain object. The running instance is shutdown if not down
- * already and all resources used by it are given back to the hypervisor.
- * The data structure is freed and should not be used thereafter if the
- * call does not return an error.
- * This function may requires privileged access
+ * already and all resources used by it are given back to the hypervisor. This
+ * does not free the associated virDomainPtr object.
+ * This function may require privileged access
*
* Returns 0 in case of success and -1 in case of failure.
*/
@@ -3502,10 +3501,9 @@ virNetworkCreate(virNetworkPtr network)
* @network: a network object
*
* Destroy the network object. The running instance is shutdown if not down
- * already and all resources used by it are given back to the hypervisor.
- * The data structure is freed and should not be used thereafter if the
- * call does not return an error.
- * This function may requires privileged access
+ * already and all resources used by it are given back to the hypervisor. This
+ * does not free the associated virNetworkPtr object.
+ * This function may require privileged access
*
* Returns 0 in case of success and -1 in case of failure.
*/
diff --git a/src/virsh.c b/src/virsh.c
index 45af630..234fc36 100644
--- a/src/virsh.c
+++ b/src/virsh.c
@@ -1468,9 +1468,9 @@ cmdDestroy(vshControl * ctl, vshCmd * cmd)
} else {
vshError(ctl, FALSE, _("Failed to destroy domain %s"), name);
ret = FALSE;
- virDomainFree(dom);
}
+ virDomainFree(dom);
return ret;
}
@@ -2433,9 +2433,9 @@ cmdNetworkDestroy(vshControl * ctl, vshCmd * cmd)
} else {
vshError(ctl, FALSE, _("Failed to destroy network %s"), name);
ret = FALSE;
- virNetworkFree(network);
}
+ virNetworkFree(network);
return ret;
}
@@ -3161,9 +3161,9 @@ cmdPoolDestroy(vshControl * ctl, vshCmd * cmd)
} else {
vshError(ctl, FALSE, _("Failed to destroy pool %s"), name);
ret = FALSE;
- virStoragePoolFree(pool);
}
+ virStoragePoolFree(pool);
return ret;
}