[libvirt] [PATCH 0/6] Fix couple of memleaks
Couple of patches I had lying around. I expect 4/6 to be controversial a bit. But it shouldn't be a show stopper for others. Michal Privoznik (6): networkBuildDhcpDaemonCommandLine: Don't leak @configstr networkBuildDhcpDaemonCommandLine: Don't leak @configfile virPCIDeviceBindToStub: Remove unused @oldDriverPath and @oldDriverName virThreadPoolFree: Join worker threads virDomainEventCallbackListFree: Don't leak @list->callbacks qemuProcessReconnectHelper: Don't create joinable thread src/conf/domain_event.c | 1 + src/network/bridge_driver.c | 2 ++ src/qemu/qemu_process.c | 2 +- src/util/virpci.c | 4 ---- src/util/virthreadpool.c | 9 +++++++++ 5 files changed, 13 insertions(+), 5 deletions(-) -- 1.8.3.2
In the function an helper to build the dnsmasq config file is used. The content to be stored then into the config file is kept in @configstr variable. However, once written, it's never freed and as soon as the control reaches the 'return' line, the variable (and hence allocated memory handle) will go off the scope and is leaked. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 3423a45..fd4dfc9 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1061,6 +1061,7 @@ networkBuildDhcpDaemonCommandLine(virNetworkObjPtr network, *cmdout = cmd; ret = 0; cleanup: + VIR_FREE(configstr); return ret; } -- 1.8.3.2
On Thu, Nov 14, 2013 at 10:51:11AM +0100, Michal Privoznik wrote:
In the function an helper to build the dnsmasq config file is used. The content to be stored then into the config file is kept in @configstr variable. However, once written, it's never freed and as soon as the control reaches the 'return' line, the variable (and hence allocated memory handle) will go off the scope and is leaked.
Vary hard to parse, I'd say $SUBJ is enough.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 3423a45..fd4dfc9 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1061,6 +1061,7 @@ networkBuildDhcpDaemonCommandLine(virNetworkObjPtr network, *cmdout = cmd; ret = 0; cleanup: + VIR_FREE(configstr); return ret; }
-- 1.8.3.2
ACK, Martin
The name of the dnsmasq's config file is initialized and passed to virCommand. However, the virCommand creates a copy, so we are responsible for freeing the @configfile ourselves. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index fd4dfc9..4298576 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1061,6 +1061,7 @@ networkBuildDhcpDaemonCommandLine(virNetworkObjPtr network, *cmdout = cmd; ret = 0; cleanup: + VIR_FREE(configfile); VIR_FREE(configstr); return ret; } -- 1.8.3.2
On Thu, Nov 14, 2013 at 10:51:12AM +0100, Michal Privoznik wrote:
The name of the dnsmasq's config file is initialized and passed to virCommand. However, the virCommand creates a copy, so we are responsible for freeing the @configfile ourselves.
Squashing with 1/1 would've been fine too ;) ACK, Martin
These two chunks had to be part of df4283a55bf. But for some unclear reason, the weren't. Anyway, these two variables are not used anywhere within function. They're initialized to NULL and then VIR_FREE()-d. And there's no reason do do two NOPs, right? Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virpci.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/util/virpci.c b/src/util/virpci.c index a41a22b..8a3a492 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1085,8 +1085,6 @@ virPCIDeviceBindToStub(virPCIDevicePtr dev, char *stubDriverPath = NULL; char *driverLink = NULL; - char *oldDriverPath = NULL; - char *oldDriverName = NULL; char *path = NULL; /* reused for different purposes */ const char *newDriverName = NULL; @@ -1217,8 +1215,6 @@ remove_id: cleanup: VIR_FREE(stubDriverPath); VIR_FREE(driverLink); - VIR_FREE(oldDriverPath); - VIR_FREE(oldDriverName); VIR_FREE(path); if (newDriverName && -- 1.8.3.2
On Thu, Nov 14, 2013 at 10:51:13AM +0100, Michal Privoznik wrote:
These two chunks had to be part of df4283a55bf. But for some unclear reason, the weren't. Anyway, these two variables are not used anywhere within function. They're initialized to NULL and then VIR_FREE()-d. And there's no reason do do two NOPs, right?
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virpci.c | 4 ---- 1 file changed, 4 deletions(-)
ACK, Martin
Even though currently we are freeing the pool of worker threads at the daemon very end, nothing holds us back in joining the worker threads. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virthreadpool.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/util/virthreadpool.c b/src/util/virthreadpool.c index b1e2c0c..99f36ec 100644 --- a/src/util/virthreadpool.c +++ b/src/util/virthreadpool.c @@ -241,6 +241,9 @@ void virThreadPoolFree(virThreadPoolPtr pool) { virThreadPoolJobPtr job; bool priority = false; + size_t i; + size_t nWorkers = pool->nWorkers; + size_t nPrioWorkers = pool->nPrioWorkers; if (!pool) return; @@ -262,6 +265,12 @@ void virThreadPoolFree(virThreadPoolPtr pool) VIR_FREE(job); } + for (i = 0; i < nWorkers; i++) + virThreadJoin(&pool->workers[i]); + + for (i = 0; i < nPrioWorkers; i++) + virThreadJoin(&pool->prioWorkers[i]); + VIR_FREE(pool->workers); virMutexUnlock(&pool->mutex); virMutexDestroy(&pool->mutex); -- 1.8.3.2
On Thu, Nov 14, 2013 at 10:51:14AM +0100, Michal Privoznik wrote:
Even though currently we are freeing the pool of worker threads at the daemon very end, nothing holds us back in joining the worker threads.
s/daemon/daemon's/ ??? Unless the thread is blocked, right? Not that it should happen, but maybe adding a wrapper for pthread_timedjoin_np and using that (if available) might be better? Just an idea, I haven't thought that through ;)
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virthreadpool.c | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/src/util/virthreadpool.c b/src/util/virthreadpool.c index b1e2c0c..99f36ec 100644 --- a/src/util/virthreadpool.c +++ b/src/util/virthreadpool.c @@ -241,6 +241,9 @@ void virThreadPoolFree(virThreadPoolPtr pool) { virThreadPoolJobPtr job; bool priority = false; + size_t i; + size_t nWorkers = pool->nWorkers; + size_t nPrioWorkers = pool->nPrioWorkers;
if (!pool) return; @@ -262,6 +265,12 @@ void virThreadPoolFree(virThreadPoolPtr pool) VIR_FREE(job); }
+ for (i = 0; i < nWorkers; i++) + virThreadJoin(&pool->workers[i]); + + for (i = 0; i < nPrioWorkers; i++) + virThreadJoin(&pool->prioWorkers[i]); + VIR_FREE(pool->workers); virMutexUnlock(&pool->mutex); virMutexDestroy(&pool->mutex); -- 1.8.3.2
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 11/14/2013 02:51 AM, Michal Privoznik wrote:
Even though currently we are freeing the pool of worker threads at the daemon very end, nothing holds us back in joining the worker threads.
Would it be better to detach the threads than to wait for them to join? -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Thu, Nov 14, 2013 at 07:13:43AM -0700, Eric Blake wrote:
On 11/14/2013 02:51 AM, Michal Privoznik wrote:
Even though currently we are freeing the pool of worker threads at the daemon very end, nothing holds us back in joining the worker threads.
Would it be better to detach the threads than to wait for them to join?
There's code earlier which uses a condition variable wait for them to actually exit, so IIUC, joining them shouldn't add any delay. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
The @list->callbacks is an array that is inflated whenever a new event is added, e.g. via virDomainEventCallbackListAddID(). However, when we are freeing the array, we free the items within it but forgot to actually free it. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_event.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c index 16ae92b..19e3920 100644 --- a/src/conf/domain_event.c +++ b/src/conf/domain_event.c @@ -147,6 +147,7 @@ virDomainEventCallbackListFree(virDomainEventCallbackListPtr list) (*freecb)(list->callbacks[i]->opaque); VIR_FREE(list->callbacks[i]); } + VIR_FREE(list->callbacks); VIR_FREE(list); } -- 1.8.3.2
On Thu, Nov 14, 2013 at 10:51:15AM +0100, Michal Privoznik wrote:
The @list->callbacks is an array that is inflated whenever a new event is added, e.g. via virDomainEventCallbackListAddID(). However, when we are freeing the array, we free the items within it but forgot to actually free it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_event.c | 1 + 1 file changed, 1 insertion(+)
ACK, Martin
In the qemuProcessReconnectHelper() a new thread that do all the interesting work is spawned. The rationale is to not block the daemon startup process in case of unresponsive qemu. However, the thread handler is a local variable which gets lost once the control goes out of scope. Hence, the thread gets leaked. We can avoid this if the thread isn't make joinable. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_process.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index e34f542..f698d47 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3281,7 +3281,7 @@ qemuProcessReconnectHelper(virDomainObjPtr obj, */ virConnectRef(data->conn); - if (virThreadCreate(&thread, true, qemuProcessReconnect, data) < 0) { + if (virThreadCreate(&thread, false, qemuProcessReconnect, data) < 0) { virConnectClose(data->conn); -- 1.8.3.2
On Thu, Nov 14, 2013 at 10:51:16AM +0100, Michal Privoznik wrote:
In the qemuProcessReconnectHelper() a new thread that do all the
s/do/does/
interesting work is spawned. The rationale is to not block the daemon startup process in case of unresponsive qemu. However, the thread handler is a local variable which gets lost once the control goes out of scope. Hence, the thread gets leaked. We can avoid this if the thread
s/,// ???
isn't make joinable.
s/make/made/
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_process.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ACK, Martin
On 14.11.2013 12:55, Martin Kletzander wrote:
On Thu, Nov 14, 2013 at 10:51:16AM +0100, Michal Privoznik wrote:
In the qemuProcessReconnectHelper() a new thread that do all the
s/do/does/
interesting work is spawned. The rationale is to not block the daemon startup process in case of unresponsive qemu. However, the thread handler is a local variable which gets lost once the control goes out of scope. Hence, the thread gets leaked. We can avoid this if the thread
s/,// ???
isn't make joinable.
s/make/made/
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_process.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ACK,
Martin
Thanks. I've pushed the patches except for 4/6 which we doesn't have a clear consensus on yet. Michal
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Martin Kletzander -
Michal Privoznik