10:59 a.m.
These patches resolve a Coverity warning and a found Valgrind leak.
Added more patterns to the .valgrind.supp file to suppress the commandtests
and seclabeltest output. This will thus result in a mostly clean valgrind
run. All that remains is a hotplugtest failure, which some patches were posted:
https://www.redhat.com/archives/libvir-list/2013-July/msg01156.html
and
https://www.redhat.com/archives/libvir-list/2013-July/msg01158.html
but did not completely resolve the failures.
John Ferlan (3):
lxc: Resolve Coverity warning
domain_event: Resolve memory leak found by Valgrind
valgrind.supp: Add more valgrind suppression paths
src/conf/domain_event.c | 3 +++
src/lxc/lxc_process.c | 2 +-
tests/.valgrind.supp | 60 +++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 64 insertions(+), 1 deletion(-)
--
1.8.1.4
10:59 a.m.
New subject: [libvirt] [PATCH 1/3] lxc: Resolve Coverity warning
Commit 'c8695053' resulted in the following:
Coverity error seen in the output:
ERROR: REVERSE_INULL
FUNCTION: lxcProcessAutoDestroy
Due to the 'dom' being checked before 'dom->persistent' since
'dom'
is already dereferenced prior to that.
---
src/lxc/lxc_process.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
index 5b83ccb..5c4f8c8 100644
--- a/src/lxc/lxc_process.c
+++ b/src/lxc/lxc_process.c
@@ -74,7 +74,7 @@ lxcProcessAutoDestroy(virDomainObjPtr dom,
VIR_DOMAIN_EVENT_STOPPED_DESTROYED);
priv->doneStopEvent = true;
- if (dom && !dom->persistent) {
+ if (!dom->persistent) {
virDomainObjListRemove(driver->domains, dom);
dom = NULL;
}
--
1.8.1.4
11:10 a.m.
New subject: [libvirt] [PATCH 1/3] lxc: Resolve Coverity warning
On 07/23/13 17:59, John Ferlan wrote:
Commit 'c8695053' resulted in the following:
Coverity error seen in the output:
ERROR: REVERSE_INULL
FUNCTION: lxcProcessAutoDestroy
Due to the 'dom' being checked before 'dom->persistent' since
'dom'
is already dereferenced prior to that.
---
src/lxc/lxc_process.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
index 5b83ccb..5c4f8c8 100644
--- a/src/lxc/lxc_process.c
+++ b/src/lxc/lxc_process.c
@@ -74,7 +74,7 @@ lxcProcessAutoDestroy(virDomainObjPtr dom,
VIR_DOMAIN_EVENT_STOPPED_DESTROYED);
priv->doneStopEvent = true;
- if (dom && !dom->persistent) {
+ if (!dom->persistent) {
virDomainObjListRemove(driver->domains, dom);
dom = NULL;
}
ACK, dom is indeed dereferenced before, thus it will not crash here.
Peter
10:59 a.m.
New subject: [libvirt] [PATCH 2/3] domain_event: Resolve memory leak found by Valgrind
Commit id '4421e257' strdup'd devAlias, but didn't free
Running qemuhotplugtest under valgrind resulted in the following:
==7375== 9 bytes in 1 blocks are definitely lost in loss record 11 of 70
==7375== at 0x4A0887C: malloc (vg_replace_malloc.c:270)
==7375== by 0x37C1085D71: strdup (strdup.c:42)
==7375== by 0x4CBBD5F: virStrdup (virstring.c:554)
==7375== by 0x4CFF9CB: virDomainEventDeviceRemovedNew (domain_event.c:1174)
==7375== by 0x427791: qemuDomainRemoveChrDevice (qemu_hotplug.c:2508)
==7375== by 0x42C65D: qemuDomainDetachChrDevice (qemu_hotplug.c:3357)
==7375== by 0x41C94F: testQemuHotplug (qemuhotplugtest.c:115)
==7375== by 0x41D817: virtTestRun (testutils.c:168)
==7375== by 0x41C400: mymain (qemuhotplugtest.c:322)
==7375== by 0x41DF3A: virtTestMain (testutils.c:764)
==7375== by 0x37C1021A04: (below main) (libc-start.c:225)
---
src/conf/domain_event.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c
index 640463c..16ae92b 100644
--- a/src/conf/domain_event.c
+++ b/src/conf/domain_event.c
@@ -520,6 +520,9 @@ void virDomainEventFree(virDomainEventPtr event)
case VIR_DOMAIN_EVENT_ID_TRAY_CHANGE:
VIR_FREE(event->data.trayChange.devAlias);
break;
+ case VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED:
+ VIR_FREE(event->data.deviceRemoved.devAlias);
+ break;
}
VIR_FREE(event->dom.name);
--
1.8.1.4
11:06 a.m.
New subject: [libvirt] [PATCH 2/3] domain_event: Resolve memory leak found by Valgrind
On 07/23/13 17:59, John Ferlan wrote:
Commit id '4421e257' strdup'd devAlias, but didn't
free
Running qemuhotplugtest under valgrind resulted in the following:
==7375== 9 bytes in 1 blocks are definitely lost in loss record 11 of 70
==7375== at 0x4A0887C: malloc (vg_replace_malloc.c:270)
==7375== by 0x37C1085D71: strdup (strdup.c:42)
==7375== by 0x4CBBD5F: virStrdup (virstring.c:554)
==7375== by 0x4CFF9CB: virDomainEventDeviceRemovedNew (domain_event.c:1174)
==7375== by 0x427791: qemuDomainRemoveChrDevice (qemu_hotplug.c:2508)
==7375== by 0x42C65D: qemuDomainDetachChrDevice (qemu_hotplug.c:3357)
==7375== by 0x41C94F: testQemuHotplug (qemuhotplugtest.c:115)
==7375== by 0x41D817: virtTestRun (testutils.c:168)
==7375== by 0x41C400: mymain (qemuhotplugtest.c:322)
==7375== by 0x41DF3A: virtTestMain (testutils.c:764)
==7375== by 0x37C1021A04: (below main) (libc-start.c:225)
---
src/conf/domain_event.c | 3 +++
1 file changed, 3 insertions(+)
ACK.
Peter
10:59 a.m.
New subject: [libvirt] [PATCH 3/3] valgrind.supp: Add more valgrind suppression paths
Update based on recent run/failures seen
---
tests/.valgrind.supp | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 60 insertions(+)
diff --git a/tests/.valgrind.supp b/tests/.valgrind.supp
index 10cc3c0..f04912d 100644
--- a/tests/.valgrind.supp
+++ b/tests/.valgrind.supp
@@ -75,3 +75,63 @@
...
obj:*/lib*/libc-2.*so*
}
+#
+# commandtest validates the various threaded commands. The
+# virThreadCreate() routine allocates and passes args to the
+# new thread which now owns the 'args' and thus cannot be free'd
+#
+{
+ commandtestLeak1
+ Memcheck:Leak
+ fun:calloc
+ fun:virAlloc
+ fun:virThreadCreate
+ fun:mymain
+ fun:virtTestMain
+}
+#
+# The Error code requires static memory that is never free'd
+# for thread local storage to store error message/data
+#
+{
+ commandtestLeak2
+ Memcheck:Leak
+ fun:calloc
+ fun:virAlloc
+ ...
+ fun:vir*LastError*
+ fun:virEventRunDefaultImpl
+ fun:virCommandThreadWorker
+ fun:virThreadHelper
+ fun:start_thread
+ fun:clone
+}
+#
+# Some of the commandtests (test0, test1, test4, & test18) cause the
+# following traceback although it appears the memory is properly freed
+#
+{
+ commandtestLeak3
+ Memcheck:Leak
+ fun:calloc
+ fun:virAllocN
+ fun:virEventPollRunOnce
+ fun:virEventRunDefaultImpl
+ fun:virCommandThreadWorker
+ fun:virThreadHelper
+ fun:start_thread
+ fun:clone
+}
+#
+# seclabeltest relies on 'selabel_close' which is not in libvirt
+#
+{
+ seclabeltestcond1
+ Memcheck:Cond
+ obj:/usr/lib64/libselinux.so.1
+ fun:selabel_close
+ fun:virSecuritySELinuxSecurityDriverClose
+ fun:virSecurityManagerDispose
+ fun:virObjectUnref
+ fun:main
+}
--
1.8.1.4
11:39 a.m.
New subject: [libvirt] [PATCH 3/3] valgrind.supp: Add more valgrind suppression paths
On 07/23/13 17:59, John Ferlan wrote:
Update based on recent run/failures seen
---
tests/.valgrind.supp | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 60 insertions(+)
diff --git a/tests/.valgrind.supp b/tests/.valgrind.supp
index 10cc3c0..f04912d 100644
--- a/tests/.valgrind.supp
+++ b/tests/.valgrind.supp
I'm not an valgrind expert, but those look okay to me.
ACK.
Peter
11:45 a.m.
New subject: [libvirt] [PATCH 3/3] valgrind.supp: Add more valgrind suppression paths
On 07/23/2013 09:59 AM, John Ferlan wrote:
Update based on recent run/failures seen
---
tests/.valgrind.supp | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 60 insertions(+)
diff --git a/tests/.valgrind.supp b/tests/.valgrind.supp
index 10cc3c0..f04912d 100644
--- a/tests/.valgrind.supp
+++ b/tests/.valgrind.supp
@@ -75,3 +75,63 @@
...
obj:*/lib*/libc-2.*so*
}
+#
+# commandtest validates the various threaded commands. The
+# virThreadCreate() routine allocates and passes args to the
+# new thread which now owns the 'args' and thus cannot be free'd
+#
+{
+ commandtestLeak1
+ Memcheck:Leak
+ fun:calloc
+ fun:virAlloc
+ fun:virThreadCreate
+ fun:mymain
+ fun:virtTestMain
Can't we call VIR_FREE(args) in the forked process? Or is this one of
those inconsequential leaks right before we exec in the child process?
+}
+#
+# The Error code requires static memory that is never free'd
+# for thread local storage to store error message/data
+#
+{
+ commandtestLeak2
+ Memcheck:Leak
+ fun:calloc
+ fun:virAlloc
+ ...
+ fun:vir*LastError*
+ fun:virEventRunDefaultImpl
+ fun:virCommandThreadWorker
+ fun:virThreadHelper
+ fun:start_thread
+ fun:clone
I thought we had the ability to wire up destructors for thread local
storage, that gets called when the thread completes?
At any rate, since all of these suppressions are solely for test files,
we aren't masking leaks in libvirtd proper, so if it makes it easier to
spot the introduction of new leaks, I'm okay with this patch as-is.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
3 p.m.
New subject: [libvirt] [PATCH 3/3] valgrind.supp: Add more valgrind suppression paths
On 07/23/2013 12:45 PM, Eric Blake wrote:
On 07/23/2013 09:59 AM, John Ferlan wrote:
> Update based on recent run/failures seen
> ---
> tests/.valgrind.supp | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 60 insertions(+)
>
> diff --git a/tests/.valgrind.supp b/tests/.valgrind.supp
> index 10cc3c0..f04912d 100644
> --- a/tests/.valgrind.supp
> +++ b/tests/.valgrind.supp
> @@ -75,3 +75,63 @@
> ...
> obj:*/lib*/libc-2.*so*
> }
> +#
> +# commandtest validates the various threaded commands. The
> +# virThreadCreate() routine allocates and passes args to the
> +# new thread which now owns the 'args' and thus cannot be free'd
> +#
> +{
> + commandtestLeak1
> + Memcheck:Leak
> + fun:calloc
> + fun:virAlloc
> + fun:virThreadCreate
> + fun:mymain
> + fun:virtTestMain
Can't we call VIR_FREE(args) in the forked process? Or is this one of
those inconsequential leaks right before we exec in the child process?
We do call VIR_FREE() in virThreadHelper() from the pthread_create().
> +}
> +#
> +# The Error code requires static memory that is never free'd
> +# for thread local storage to store error message/data
> +#
> +{
> + commandtestLeak2
> + Memcheck:Leak
> + fun:calloc
> + fun:virAlloc
> + ...
> + fun:vir*LastError*
> + fun:virEventRunDefaultImpl
> + fun:virCommandThreadWorker
> + fun:virThreadHelper
> + fun:start_thread
> + fun:clone
I thought we had the ability to wire up destructors for thread local
storage, that gets called when the thread completes?
The LastError code does wire up the destructor from how I read it.
It's very strange - running the same ranges of tests two times in a row
had different results... That is, if I did "make -C tests valgrind
TESTS=commandtest VIR_TEST_RANGE=1-4" two times in a row - one time I'd
get 1 set of tracebacks and the next time I get 2. If I run just one
test (1-1), I get none. I'm not sure I completely understand why
At any rate, since all of these suppressions are solely for test
files,
we aren't masking leaks in libvirtd proper, so if it makes it easier to
spot the introduction of new leaks, I'm okay with this patch as-is.
Strange - I thought commandtestLeak3 was the most odd - only 4 tests
cause issues, but "sometimes" running them singly using VIR_TEST_RANGE
resulted in no leak shown.
I assume the failures were related to perhaps the "timing" or
"synchronization" between determining which thread was doing what and
how quickly or slowly it did it.
I can hold off on patch 3 if that's desired, too.
John
4177
days inactive
4177
days old
8 comments
3 participants
participants (3)
-
Eric Blake -
John Ferlan -
Peter Krempa