2:37 a.m.
*** BLURB HERE ***
Michal Prívozník (5):
domaincapstest: Don't leak cpu definitions
testutilsxen: Avoid double free of driver caps
virCapabilitiesHostNUMAUnref: Accept NULL
qemu: Reoder cleanup in qemuStateCleanup()
qemu: Don't leak hostcpu or hostnuma on driver cleanup
src/conf/capabilities.c | 3 +++
src/qemu/qemu_driver.c | 45 +++++++++++++++++------------------------
tests/domaincapstest.c | 2 +-
tests/testutilsxen.c | 1 -
4 files changed, 23 insertions(+), 28 deletions(-)
--
2.24.1
2:37 a.m.
New subject: [libvirt] [PATCH 1/5] domaincapstest: Don't leak cpu definitions
When generating domain capabilities, we need to fake host CPU to
get reproducible result. We do this by copying a pre-existent CPU
config and setting VIR_TEST_MOCK_FAKE_HOST_CPU env variable which
is then consumed by qemucpumock. However, we forget to free the
CPU copy afterwards.
2,196 (2,016 direct, 180 indirect) bytes in 18 blocks are definitely lost in loss record
291 of 297
at 0x4838B86: calloc (vg_replace_malloc.c:762)
by 0x57CB6A0: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.6000.7)
by 0x4A0F72D: virCPUDefNew (cpu_conf.c:87)
by 0x4A0FAC7: virCPUDefCopyWithoutModel (cpu_conf.c:235)
by 0x4A0FBBE: virCPUDefCopy (cpu_conf.c:273)
by 0x10E3C0: testUtilsHostCpusGetDefForArch (testutilshostcpus.h:157)
by 0x10E3C0: fakeHostCPU (domaincapstest.c:61)
by 0x10E3C0: fillQemuCaps (domaincapstest.c:86)
by 0x10E3C0: test_virDomainCapsFormat (domaincapstest.c:234)
by 0x10F4BC: virTestRun (testutils.c:146)
by 0x10DE93: doTestQemuInternal (domaincapstest.c:301)
by 0x10E13D: doTestQemu (domaincapstest.c:332)
by 0x1124CF: testQemuCapsIterate (testutilsqemu.c:635)
by 0x10DCE3: mymain (domaincapstest.c:435)
by 0x10FD8B: virTestMain (testutils.c:916)
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
tests/domaincapstest.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/domaincapstest.c b/tests/domaincapstest.c
index a4a443b1d6..9f5eab3230 100644
--- a/tests/domaincapstest.c
+++ b/tests/domaincapstest.c
@@ -56,7 +56,7 @@ fillStringValues(virDomainCapsStringValuesPtr values, ...)
static int
fakeHostCPU(virArch arch)
{
- virCPUDefPtr cpu;
+ g_autoptr(virCPUDef) cpu = NULL;
if (!(cpu = testUtilsHostCpusGetDefForArch(arch))) {
virReportError(VIR_ERR_INTERNAL_ERROR,
--
2.24.1
2:37 a.m.
New subject: [libvirt] [PATCH 2/5] testutilsxen: Avoid double free of driver caps
In testXLInitDriver() a dummy driver structure is filled and it
is freed later in testXLFreeDriver(). However, it is sufficient
to unref just driver->config because that results in
libxlDriverConfigDispose() being called which unrefs
driver->config->caps. There is no need to unref it again in
testXLFreeDriver() - in fact it's undesired.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
tests/testutilsxen.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/tests/testutilsxen.c b/tests/testutilsxen.c
index 75cd42ec43..b73c79581d 100644
--- a/tests/testutilsxen.c
+++ b/tests/testutilsxen.c
@@ -105,7 +105,6 @@ libxlDriverPrivatePtr testXLInitDriver(void)
void testXLFreeDriver(libxlDriverPrivatePtr driver)
{
- virObjectUnref(driver->config->caps);
virObjectUnref(driver->config);
virObjectUnref(driver->xmlopt);
virMutexDestroy(&driver->lock);
--
2.24.1
2:37 a.m.
New subject: [libvirt] [PATCH 3/5] virCapabilitiesHostNUMAUnref: Accept NULL
Fortunately, this is not causing any problems now because glib
does this check for us when calling this function via attribute
cleanup. But in future commit we will explicitly call this
function over a struct member that might be NULL.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/capabilities.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
index 4fac59e6f7..a782d92956 100644
--- a/src/conf/capabilities.c
+++ b/src/conf/capabilities.c
@@ -184,6 +184,9 @@ virCapabilitiesFreeStoragePool(virCapsStoragePoolPtr pool)
void
virCapabilitiesHostNUMAUnref(virCapsHostNUMAPtr caps)
{
+ if (!caps)
+ return;
+
if (g_atomic_int_dec_and_test(&caps->refs)) {
g_ptr_array_unref(caps->cells);
--
2.24.1
6:41 a.m.
New subject: [libvirt] [PATCH 3/5] virCapabilitiesHostNUMAUnref: Accept NULL
On 12/18/19 5:37 AM, Michal Privoznik wrote:
Fortunately, this is not causing any problems now because glib
does this check for us when calling this function via attribute
cleanup. But in future commit we will explicitly call this
nit: "But in a future commit ..."
2:37 a.m.
New subject: [libvirt] [PATCH 4/5] qemu: Reoder cleanup in qemuStateCleanup()
This function is supposed to clean up virQEMUDriver structure and
free individual members. However, it's doing that in random order
which makes it hard to track which members are being freed and
which are not. Do the free in reverse order than the structure
definition - assuming that the most important members (like
mutex) are declared first and freed last.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_driver.c | 43 +++++++++++++++++-------------------------
1 file changed, 17 insertions(+), 26 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 65c3be58d3..0b9e31b344 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1122,38 +1122,29 @@ qemuStateCleanup(void)
if (!qemu_driver)
return -1;
- if (qemu_driver->lockFD != -1)
- virPidFileRelease(qemu_driver->config->stateDir, "driver",
qemu_driver->lockFD);
- virThreadPoolFree(qemu_driver->workerPool);
- virObjectUnref(qemu_driver->config);
- virObjectUnref(qemu_driver->hostdevMgr);
- virHashFree(qemu_driver->sharedDevices);
- virObjectUnref(qemu_driver->caps);
- virObjectUnref(qemu_driver->qemuCapsCache);
-
- virObjectUnref(qemu_driver->domains);
- virPortAllocatorRangeFree(qemu_driver->remotePorts);
- virPortAllocatorRangeFree(qemu_driver->webSocketPorts);
- virPortAllocatorRangeFree(qemu_driver->migrationPorts);
virObjectUnref(qemu_driver->migrationErrors);
-
- virObjectUnref(qemu_driver->xmlopt);
-
- virSysinfoDefFree(qemu_driver->hostsysinfo);
-
virObjectUnref(qemu_driver->closeCallbacks);
-
- VIR_FREE(qemu_driver->qemuImgBinary);
-
+ virLockManagerPluginUnref(qemu_driver->lockManager);
+ virSysinfoDefFree(qemu_driver->hostsysinfo);
+ virPortAllocatorRangeFree(qemu_driver->migrationPorts);
+ virPortAllocatorRangeFree(qemu_driver->webSocketPorts);
+ virPortAllocatorRangeFree(qemu_driver->remotePorts);
+ virHashFree(qemu_driver->sharedDevices);
+ virObjectUnref(qemu_driver->hostdevMgr);
virObjectUnref(qemu_driver->securityManager);
-
- ebtablesContextFree(qemu_driver->ebtables);
-
- /* Free domain callback list */
virObjectUnref(qemu_driver->domainEventState);
+ virObjectUnref(qemu_driver->qemuCapsCache);
+ virObjectUnref(qemu_driver->xmlopt);
+ virObjectUnref(qemu_driver->caps);
+ ebtablesContextFree(qemu_driver->ebtables);
+ VIR_FREE(qemu_driver->qemuImgBinary);
+ virObjectUnref(qemu_driver->domains);
+ virThreadPoolFree(qemu_driver->workerPool);
- virLockManagerPluginUnref(qemu_driver->lockManager);
+ if (qemu_driver->lockFD != -1)
+ virPidFileRelease(qemu_driver->config->stateDir, "driver",
qemu_driver->lockFD);
+ virObjectUnref(qemu_driver->config);
virMutexDestroy(&qemu_driver->lock);
VIR_FREE(qemu_driver);
--
2.24.1
6:42 a.m.
New subject: [libvirt] [PATCH 4/5] qemu: Reoder cleanup in qemuStateCleanup()
Commit title has a typo: "Reoder" instead of "Reorder"
On 12/18/19 5:37 AM, Michal Privoznik wrote:
This function is supposed to clean up virQEMUDriver structure and
free individual members. However, it's doing that in random order
which makes it hard to track which members are being freed and
which are not. Do the free in reverse order than the structure
definition - assuming that the most important members (like
mutex) are declared first and freed last.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
2:37 a.m.
New subject: [libvirt] [PATCH 5/5] qemu: Don't leak hostcpu or hostnuma on driver cleanup
When freeing qemu driver struct members, we forgot to free
@hostcpu and @hostnuma members.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_driver.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 0b9e31b344..ec8faf384c 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1135,6 +1135,8 @@ qemuStateCleanup(void)
virObjectUnref(qemu_driver->domainEventState);
virObjectUnref(qemu_driver->qemuCapsCache);
virObjectUnref(qemu_driver->xmlopt);
+ virCPUDefFree(qemu_driver->hostcpu);
+ virCapabilitiesHostNUMAUnref(qemu_driver->hostnuma);
virObjectUnref(qemu_driver->caps);
ebtablesContextFree(qemu_driver->ebtables);
VIR_FREE(qemu_driver->qemuImgBinary);
--
2.24.1
6:44 a.m.
On 12/18/19 5:37 AM, Michal Privoznik wrote:
*** BLURB HERE ***
Michal Prívozník (5):
domaincapstest: Don't leak cpu definitions
testutilsxen: Avoid double free of driver caps
virCapabilitiesHostNUMAUnref: Accept NULL
qemu: Reoder cleanup in qemuStateCleanup()
qemu: Don't leak hostcpu or hostnuma on driver cleanup
Just a couple of grammatical nits in 2 patches.
For all patches:
Reviewed-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
1804
days inactive
1804
days old
8 comments
2 participants
participants (2)
-
Daniel Henrique Barboza -
Michal Privoznik