[libvirt] [PATCH 0/3] Fix more memory leaks
Ideally, these would be merged before the release. But I don't have strong opinion on that. Michal Privoznik (3): virDomainDefParseXML: Free @tmp when parsing genid qemuxml2argvtest: Don't initialize qemuCaps twice virQEMUCapsSetHostModel: Free cpuData before setting it src/conf/domain_conf.c | 1 + src/qemu/qemu_capabilities.c | 4 ++++ tests/qemuxml2argvtest.c | 2 -- 3 files changed, 5 insertions(+), 2 deletions(-) -- 2.16.1
We need to free return value of virXPathString(). ==12962== 37 bytes in 1 blocks are definitely lost in loss record 156 of 331 ==12962== at 0x4C2AF0F: malloc (vg_replace_malloc.c:299) ==12962== by 0x91E8439: strdup (in /lib64/libc-2.25.so) ==12962== by 0x5DBD551: virStrdup (virstring.c:977) ==12962== by 0x5DD3E5E: virXPathString (virxml.c:84) ==12962== by 0x5E178AB: virDomainDefParseXML (domain_conf.c:19110) ==12962== by 0x5E1E985: virDomainDefParseNode (domain_conf.c:20885) ==12962== by 0x5E1E7CB: virDomainDefParse (domain_conf.c:20827) ==12962== by 0x5E1E871: virDomainDefParseFile (domain_conf.c:20853) Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 27e2bd50eb..d8b4fbd6b0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -19120,6 +19120,7 @@ virDomainDefParseXML(xmlDocPtr xml, "%s", _("malformed genid element")); goto error; } + VIR_FREE(tmp); } } VIR_FREE(nodes); -- 2.16.1
On Wed, May 30, 2018 at 18:04:27 +0200, Michal Privoznik wrote:
We need to free return value of virXPathString().
==12962== 37 bytes in 1 blocks are definitely lost in loss record 156 of 331 ==12962== at 0x4C2AF0F: malloc (vg_replace_malloc.c:299) ==12962== by 0x91E8439: strdup (in /lib64/libc-2.25.so) ==12962== by 0x5DBD551: virStrdup (virstring.c:977) ==12962== by 0x5DD3E5E: virXPathString (virxml.c:84) ==12962== by 0x5E178AB: virDomainDefParseXML (domain_conf.c:19110) ==12962== by 0x5E1E985: virDomainDefParseNode (domain_conf.c:20885) ==12962== by 0x5E1E7CB: virDomainDefParse (domain_conf.c:20827) ==12962== by 0x5E1E871: virDomainDefParseFile (domain_conf.c:20853)
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 27e2bd50eb..d8b4fbd6b0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -19120,6 +19120,7 @@ virDomainDefParseXML(xmlDocPtr xml, "%s", _("malformed genid element")); goto error; } + VIR_FREE(tmp);
ACK
On 05/30/2018 06:46 PM, Peter Krempa wrote:
On Wed, May 30, 2018 at 18:04:27 +0200, Michal Privoznik wrote:
We need to free return value of virXPathString().
==12962== 37 bytes in 1 blocks are definitely lost in loss record 156 of 331 ==12962== at 0x4C2AF0F: malloc (vg_replace_malloc.c:299) ==12962== by 0x91E8439: strdup (in /lib64/libc-2.25.so) ==12962== by 0x5DBD551: virStrdup (virstring.c:977) ==12962== by 0x5DD3E5E: virXPathString (virxml.c:84) ==12962== by 0x5E178AB: virDomainDefParseXML (domain_conf.c:19110) ==12962== by 0x5E1E985: virDomainDefParseNode (domain_conf.c:20885) ==12962== by 0x5E1E7CB: virDomainDefParse (domain_conf.c:20827) ==12962== by 0x5E1E871: virDomainDefParseFile (domain_conf.c:20853)
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 27e2bd50eb..d8b4fbd6b0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -19120,6 +19120,7 @@ virDomainDefParseXML(xmlDocPtr xml, "%s", _("malformed genid element")); goto error; } + VIR_FREE(tmp);
ACK
And safe for freeze? ;-) This one fixes a feature that was introduced in this release. The rest of the patches can go after the release. Michal
On Thu, May 31, 2018 at 09:16:10 +0200, Michal Privoznik wrote:
On 05/30/2018 06:46 PM, Peter Krempa wrote:
On Wed, May 30, 2018 at 18:04:27 +0200, Michal Privoznik wrote:
[...]
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 27e2bd50eb..d8b4fbd6b0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -19120,6 +19120,7 @@ virDomainDefParseXML(xmlDocPtr xml, "%s", _("malformed genid element")); goto error; } + VIR_FREE(tmp);
ACK
And safe for freeze? ;-) This one fixes a feature that was introduced in this release. The rest of the patches can go after the release.
Yes. I thought it was obvious :)
There's no point in calling testInitQEMUCaps() (which sets info.qemuCaps) only to overwrite (and leak) it on the very next line. ==12962== 296 (208 direct, 88 indirect) bytes in 1 blocks are definitely lost in loss record 265 of 331 ==12962== at 0x4C2CF26: calloc (vg_replace_malloc.c:711) ==12962== by 0x5D28D9F: virAllocVar (viralloc.c:560) ==12962== by 0x5D96AB4: virObjectNew (virobject.c:239) ==12962== by 0x56DB7C7: virQEMUCapsNew (qemu_capabilities.c:1480) ==12962== by 0x112A5B: testInitQEMUCaps (qemuxml2argvtest.c:361) ==12962== by 0x1371C8: mymain (qemuxml2argvtest.c:2871) ==12962== by 0x13AD0B: virTestMain (testutils.c:1120) ==12962== by 0x1372FD: main (qemuxml2argvtest.c:2883) Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/qemuxml2argvtest.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index ddd2b88c0a..6f421ce8f5 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -699,8 +699,6 @@ mymain(void) (flags), parseFlags, false, NULL \ }; \ info.skipLegacyCPUs = skipLegacyCPUs; \ - if (testInitQEMUCaps(&info, gic) < 0) \ - return EXIT_FAILURE; \ if (!(info.qemuCaps = qemuTestParseCapabilitiesArch(virArchFromString(arch), \ capsfile))) \ return EXIT_FAILURE; \ -- 2.16.1
On Wed, May 30, 2018 at 18:04:28 +0200, Michal Privoznik wrote:
There's no point in calling testInitQEMUCaps() (which sets info.qemuCaps) only to overwrite (and leak) it on the very next line.
==12962== 296 (208 direct, 88 indirect) bytes in 1 blocks are definitely lost in loss record 265 of 331 ==12962== at 0x4C2CF26: calloc (vg_replace_malloc.c:711) ==12962== by 0x5D28D9F: virAllocVar (viralloc.c:560) ==12962== by 0x5D96AB4: virObjectNew (virobject.c:239) ==12962== by 0x56DB7C7: virQEMUCapsNew (qemu_capabilities.c:1480) ==12962== by 0x112A5B: testInitQEMUCaps (qemuxml2argvtest.c:361) ==12962== by 0x1371C8: mymain (qemuxml2argvtest.c:2871) ==12962== by 0x13AD0B: virTestMain (testutils.c:1120) ==12962== by 0x1372FD: main (qemuxml2argvtest.c:2883)
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/qemuxml2argvtest.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index ddd2b88c0a..6f421ce8f5 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -699,8 +699,6 @@ mymain(void) (flags), parseFlags, false, NULL \ }; \ info.skipLegacyCPUs = skipLegacyCPUs; \ - if (testInitQEMUCaps(&info, gic) < 0) \ - return EXIT_FAILURE; \
This makes the 'gic' macro argument unused. You probably need to replace it with testQemuCapsSetGIC after the caps are parsed if the parser does not do that.
if (!(info.qemuCaps = qemuTestParseCapabilitiesArch(virArchFromString(arch), \ capsfile))) \ return EXIT_FAILURE; \ -- 2.16.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 05/30/2018 06:46 PM, Peter Krempa wrote:
On Wed, May 30, 2018 at 18:04:28 +0200, Michal Privoznik wrote:
There's no point in calling testInitQEMUCaps() (which sets info.qemuCaps) only to overwrite (and leak) it on the very next line.
==12962== 296 (208 direct, 88 indirect) bytes in 1 blocks are definitely lost in loss record 265 of 331 ==12962== at 0x4C2CF26: calloc (vg_replace_malloc.c:711) ==12962== by 0x5D28D9F: virAllocVar (viralloc.c:560) ==12962== by 0x5D96AB4: virObjectNew (virobject.c:239) ==12962== by 0x56DB7C7: virQEMUCapsNew (qemu_capabilities.c:1480) ==12962== by 0x112A5B: testInitQEMUCaps (qemuxml2argvtest.c:361) ==12962== by 0x1371C8: mymain (qemuxml2argvtest.c:2871) ==12962== by 0x13AD0B: virTestMain (testutils.c:1120) ==12962== by 0x1372FD: main (qemuxml2argvtest.c:2883)
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/qemuxml2argvtest.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index ddd2b88c0a..6f421ce8f5 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -699,8 +699,6 @@ mymain(void) (flags), parseFlags, false, NULL \ }; \ info.skipLegacyCPUs = skipLegacyCPUs; \ - if (testInitQEMUCaps(&info, gic) < 0) \ - return EXIT_FAILURE; \
This makes the 'gic' macro argument unused. You probably need to replace it with testQemuCapsSetGIC after the caps are parsed if the parser does not do that.
Ah good point. However, since all callers of this macro pass GIC_NONE (if anything) the argument is not needed. The qemuCaps code is perfectly capable of handling no gic set (seevirQEMUCapsSupportsGICVersion()). Because of this I'm going to remove the argument in v2. Michal
On Thu, May 31, 2018 at 10:06:38 +0200, Michal Privoznik wrote:
On 05/30/2018 06:46 PM, Peter Krempa wrote:
On Wed, May 30, 2018 at 18:04:28 +0200, Michal Privoznik wrote:
There's no point in calling testInitQEMUCaps() (which sets info.qemuCaps) only to overwrite (and leak) it on the very next line.
==12962== 296 (208 direct, 88 indirect) bytes in 1 blocks are definitely lost in loss record 265 of 331 ==12962== at 0x4C2CF26: calloc (vg_replace_malloc.c:711) ==12962== by 0x5D28D9F: virAllocVar (viralloc.c:560) ==12962== by 0x5D96AB4: virObjectNew (virobject.c:239) ==12962== by 0x56DB7C7: virQEMUCapsNew (qemu_capabilities.c:1480) ==12962== by 0x112A5B: testInitQEMUCaps (qemuxml2argvtest.c:361) ==12962== by 0x1371C8: mymain (qemuxml2argvtest.c:2871) ==12962== by 0x13AD0B: virTestMain (testutils.c:1120) ==12962== by 0x1372FD: main (qemuxml2argvtest.c:2883)
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/qemuxml2argvtest.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index ddd2b88c0a..6f421ce8f5 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -699,8 +699,6 @@ mymain(void) (flags), parseFlags, false, NULL \ }; \ info.skipLegacyCPUs = skipLegacyCPUs; \ - if (testInitQEMUCaps(&info, gic) < 0) \ - return EXIT_FAILURE; \
This makes the 'gic' macro argument unused. You probably need to replace it with testQemuCapsSetGIC after the caps are parsed if the parser does not do that.
Ah good point. However, since all callers of this macro pass GIC_NONE (if anything) the argument is not needed. The qemuCaps code is perfectly capable of handling no gic set (seevirQEMUCapsSupportsGICVersion()). Because of this I'm going to remove the argument in v2.
Well, that was so that we could later add macros for ARM, but as we parse in capabilities, it should perhaps be encoded in them rather than provided externally. pre-emptive ACK and obvious-safe-for-freeze for the version with the GIC argument removed.
Michal
While this leak happens in tests only, it is still worth fixing. ==12962== 2,035 (104 direct, 1,931 indirect) bytes in 1 blocks are definitely lost in loss record 325 of 331 ==12962== at 0x4C2CF26: calloc (vg_replace_malloc.c:711) ==12962== by 0x5D285D5: virAlloc (viralloc.c:144) ==12962== by 0x5E823EB: virCPUGetHost (cpu.c:411) ==12962== by 0x56DE953: virQEMUCapsInitHostCPUModel (qemu_capabilities.c:2874) ==12962== by 0x56E062F: virQEMUCapsLoadCache (qemu_capabilities.c:3435) ==12962== by 0x13802F: qemuTestParseCapabilitiesArch (testutilsqemu.c:500) ==12962== by 0x1371F0: mymain (qemuxml2argvtest.c:2871) ==12962== by 0x13AD0B: virTestMain (testutils.c:1120) ==12962== by 0x1372FD: main (qemuxml2argvtest.c:2883) Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_capabilities.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index e2e76e4dd8..9ec9089d5b 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -1857,6 +1857,10 @@ virQEMUCapsSetHostModel(virQEMUCapsPtr qemuCaps, { virQEMUCapsHostCPUDataPtr cpuData = virQEMUCapsGetHostCPUData(qemuCaps, type); + virCPUDefFree(cpuData->reported); + virCPUDefFree(cpuData->migratable); + virCPUDefFree(cpuData->full); + cpuData->reported = reported; cpuData->migratable = migratable; cpuData->full = full; -- 2.16.1
On Wed, May 30, 2018 at 18:04:29 +0200, Michal Privoznik wrote:
While this leak happens in tests only, it is still worth fixing.
==12962== 2,035 (104 direct, 1,931 indirect) bytes in 1 blocks are definitely lost in loss record 325 of 331 ==12962== at 0x4C2CF26: calloc (vg_replace_malloc.c:711) ==12962== by 0x5D285D5: virAlloc (viralloc.c:144) ==12962== by 0x5E823EB: virCPUGetHost (cpu.c:411) ==12962== by 0x56DE953: virQEMUCapsInitHostCPUModel (qemu_capabilities.c:2874) ==12962== by 0x56E062F: virQEMUCapsLoadCache (qemu_capabilities.c:3435) ==12962== by 0x13802F: qemuTestParseCapabilitiesArch (testutilsqemu.c:500) ==12962== by 0x1371F0: mymain (qemuxml2argvtest.c:2871) ==12962== by 0x13AD0B: virTestMain (testutils.c:1120) ==12962== by 0x1372FD: main (qemuxml2argvtest.c:2883)
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_capabilities.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index e2e76e4dd8..9ec9089d5b 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -1857,6 +1857,10 @@ virQEMUCapsSetHostModel(virQEMUCapsPtr qemuCaps, { virQEMUCapsHostCPUDataPtr cpuData = virQEMUCapsGetHostCPUData(qemuCaps, type);
+ virCPUDefFree(cpuData->reported); + virCPUDefFree(cpuData->migratable); + virCPUDefFree(cpuData->full);
This looks fishy. The test code always unrefs the qemuCaps object, so there should be no leak. Could you please elaborate on how this happens? My problem is that this is in the setter code.
On 05/30/2018 06:57 PM, Peter Krempa wrote:
On Wed, May 30, 2018 at 18:04:29 +0200, Michal Privoznik wrote:
While this leak happens in tests only, it is still worth fixing.
==12962== 2,035 (104 direct, 1,931 indirect) bytes in 1 blocks are definitely lost in loss record 325 of 331 ==12962== at 0x4C2CF26: calloc (vg_replace_malloc.c:711) ==12962== by 0x5D285D5: virAlloc (viralloc.c:144) ==12962== by 0x5E823EB: virCPUGetHost (cpu.c:411) ==12962== by 0x56DE953: virQEMUCapsInitHostCPUModel (qemu_capabilities.c:2874) ==12962== by 0x56E062F: virQEMUCapsLoadCache (qemu_capabilities.c:3435) ==12962== by 0x13802F: qemuTestParseCapabilitiesArch (testutilsqemu.c:500) ==12962== by 0x1371F0: mymain (qemuxml2argvtest.c:2871) ==12962== by 0x13AD0B: virTestMain (testutils.c:1120) ==12962== by 0x1372FD: main (qemuxml2argvtest.c:2883)
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_capabilities.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index e2e76e4dd8..9ec9089d5b 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -1857,6 +1857,10 @@ virQEMUCapsSetHostModel(virQEMUCapsPtr qemuCaps, { virQEMUCapsHostCPUDataPtr cpuData = virQEMUCapsGetHostCPUData(qemuCaps, type);
+ virCPUDefFree(cpuData->reported); + virCPUDefFree(cpuData->migratable); + virCPUDefFree(cpuData->full);
This looks fishy. The test code always unrefs the qemuCaps object, so there should be no leak. Could you please elaborate on how this happens?
Thing is, qemuTestParseCapabilitiesArch() calls virQEMUCapsLoadCache() (which exists only for sake of tests) which does two things: 1) roughly in the middle it calls virQEMUCapsLoadHostCPUModelInfo() where cpuData is firstly allocated 2) at the end it calls virQEMUCapsInitHostCPUModel() which overwrites whatever was loaded in 1).
My problem is that this is in the setter code.
Not at all. It's pretty common that setter frees whatever it is replacing (e.g. virQEMUCapsSetGICCapabilities()). Michal
On Thu, May 31, 2018 at 10:06:37 +0200, Michal Privoznik wrote:
On 05/30/2018 06:57 PM, Peter Krempa wrote:
On Wed, May 30, 2018 at 18:04:29 +0200, Michal Privoznik wrote:
While this leak happens in tests only, it is still worth fixing.
==12962== 2,035 (104 direct, 1,931 indirect) bytes in 1 blocks are definitely lost in loss record 325 of 331 ==12962== at 0x4C2CF26: calloc (vg_replace_malloc.c:711) ==12962== by 0x5D285D5: virAlloc (viralloc.c:144) ==12962== by 0x5E823EB: virCPUGetHost (cpu.c:411) ==12962== by 0x56DE953: virQEMUCapsInitHostCPUModel (qemu_capabilities.c:2874) ==12962== by 0x56E062F: virQEMUCapsLoadCache (qemu_capabilities.c:3435) ==12962== by 0x13802F: qemuTestParseCapabilitiesArch (testutilsqemu.c:500) ==12962== by 0x1371F0: mymain (qemuxml2argvtest.c:2871) ==12962== by 0x13AD0B: virTestMain (testutils.c:1120) ==12962== by 0x1372FD: main (qemuxml2argvtest.c:2883)
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_capabilities.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index e2e76e4dd8..9ec9089d5b 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -1857,6 +1857,10 @@ virQEMUCapsSetHostModel(virQEMUCapsPtr qemuCaps, { virQEMUCapsHostCPUDataPtr cpuData = virQEMUCapsGetHostCPUData(qemuCaps, type);
+ virCPUDefFree(cpuData->reported); + virCPUDefFree(cpuData->migratable); + virCPUDefFree(cpuData->full);
This looks fishy. The test code always unrefs the qemuCaps object, so there should be no leak. Could you please elaborate on how this happens?
Thing is, qemuTestParseCapabilitiesArch() calls virQEMUCapsLoadCache() (which exists only for sake of tests) which does two things:
1) roughly in the middle it calls virQEMUCapsLoadHostCPUModelInfo() where cpuData is firstly allocated
virQEMUCapsLoadHostCPUModelInfo calls virQEMUCapsSetCPUModelInfo which sets cpuData->info
2) at the end it calls virQEMUCapsInitHostCPUModel() which overwrites whatever was loaded in 1).
while virQEMUCapsInitHostCPUModel fills cpuData->reported, cpuData->migratable, and cpuData->full using the data from cpuData->info. It must be something else which causes the leak. Jirka
On 05/31/2018 10:57 AM, Jiri Denemark wrote:
On Thu, May 31, 2018 at 10:06:37 +0200, Michal Privoznik wrote:
On 05/30/2018 06:57 PM, Peter Krempa wrote:
On Wed, May 30, 2018 at 18:04:29 +0200, Michal Privoznik wrote:
While this leak happens in tests only, it is still worth fixing.
==12962== 2,035 (104 direct, 1,931 indirect) bytes in 1 blocks are definitely lost in loss record 325 of 331 ==12962== at 0x4C2CF26: calloc (vg_replace_malloc.c:711) ==12962== by 0x5D285D5: virAlloc (viralloc.c:144) ==12962== by 0x5E823EB: virCPUGetHost (cpu.c:411) ==12962== by 0x56DE953: virQEMUCapsInitHostCPUModel (qemu_capabilities.c:2874) ==12962== by 0x56E062F: virQEMUCapsLoadCache (qemu_capabilities.c:3435) ==12962== by 0x13802F: qemuTestParseCapabilitiesArch (testutilsqemu.c:500) ==12962== by 0x1371F0: mymain (qemuxml2argvtest.c:2871) ==12962== by 0x13AD0B: virTestMain (testutils.c:1120) ==12962== by 0x1372FD: main (qemuxml2argvtest.c:2883)
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_capabilities.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index e2e76e4dd8..9ec9089d5b 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -1857,6 +1857,10 @@ virQEMUCapsSetHostModel(virQEMUCapsPtr qemuCaps, { virQEMUCapsHostCPUDataPtr cpuData = virQEMUCapsGetHostCPUData(qemuCaps, type);
+ virCPUDefFree(cpuData->reported); + virCPUDefFree(cpuData->migratable); + virCPUDefFree(cpuData->full);
This looks fishy. The test code always unrefs the qemuCaps object, so there should be no leak. Could you please elaborate on how this happens?
Thing is, qemuTestParseCapabilitiesArch() calls virQEMUCapsLoadCache() (which exists only for sake of tests) which does two things:
1) roughly in the middle it calls virQEMUCapsLoadHostCPUModelInfo() where cpuData is firstly allocated
virQEMUCapsLoadHostCPUModelInfo calls virQEMUCapsSetCPUModelInfo which sets cpuData->info
2) at the end it calls virQEMUCapsInitHostCPUModel() which overwrites whatever was loaded in 1).
while virQEMUCapsInitHostCPUModel fills cpuData->reported, cpuData->migratable, and cpuData->full using the data from cpuData->info.
It must be something else which causes the leak.
Ah right. It's different call trace: Hardware watchpoint 2: -location qemuCaps->kvmCPU->full Old value = (virCPUDef *) 0x0 New value = (virCPUDef *) 0x5555558f1740 virQEMUCapsSetHostModel (qemuCaps=0x5555557e7ea0, type=VIR_DOMAIN_VIRT_KVM, reported=0x5555558f0770, migratable=0x5555558f1de0, full=0x5555558f1740) at qemu/qemu_capabilities.c:1863 1863 } virQEMUCapsSetHostModel 1 $ bt #0 virQEMUCapsSetHostModel (qemuCaps=0x5555557e7ea0, type=VIR_DOMAIN_VIRT_KVM, reported=0x5555558f0770, migratable=0x5555558f1de0, full=0x5555558f1740) at qemu/qemu_capabilities.c:1863 #1 0x00007ffff7211acd in virQEMUCapsInitHostCPUModel (qemuCaps=0x5555557e7ea0, hostArch=VIR_ARCH_X86_64, type=VIR_DOMAIN_VIRT_KVM) at qemu/qemu_capabilities.c:2903 #2 0x00007ffff7213630 in virQEMUCapsLoadCache (hostArch=VIR_ARCH_X86_64, qemuCaps=0x5555557e7ea0, filename=0x5555557eaef0 "tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml") at qemu/qemu_capabilities.c:3435 #3 0x0000555555584030 in qemuTestParseCapabilitiesArch (arch=VIR_ARCH_X86_64, capsFile=0x5555557eaef0 "tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml") at testutilsqemu.c:500 #4 0x000055555555fe1b in mymain () at qemuxml2argvtest.c:802 #5 0x0000555555586d0c in virTestMain (argc=1, argv=0x7fffffffd698, func=0x55555555f4f9 <mymain>) at testutils.c:1120 #6 0x00005555555832fe in main (argc=1, argv=0x7fffffffd698) at qemuxml2argvtest.c:2883 virQEMUCapsSetHostModel 1 $ c Continuing. Hardware watchpoint 2: -location qemuCaps->kvmCPU->full Old value = (virCPUDef *) 0x5555558f1740 New value = (virCPUDef *) 0x5555558f1820 virQEMUCapsSetHostModel (qemuCaps=0x5555557e7ea0, type=VIR_DOMAIN_VIRT_KVM, reported=0x5555557e2ad0, migratable=0x5555558f3190, full=0x5555558f1820) at qemu/qemu_capabilities.c:1863 1863 } virQEMUCapsSetHostModel 1 $ bt #0 virQEMUCapsSetHostModel (qemuCaps=0x5555557e7ea0, type=VIR_DOMAIN_VIRT_KVM, reported=0x5555557e2ad0, migratable=0x5555558f3190, full=0x5555558f1820) at qemu/qemu_capabilities.c:1863 #1 0x00007ffff7211acd in virQEMUCapsInitHostCPUModel (qemuCaps=0x5555557e7ea0, hostArch=VIR_ARCH_X86_64, type=VIR_DOMAIN_VIRT_KVM) at qemu/qemu_capabilities.c:2903 #2 0x000055555555eb34 in testUpdateQEMUCaps (info=0x55555579f800 <info>, vm=0x5555557e2eb0, caps=0x5555557e18b0) at qemuxml2argvtest.c:391 #3 0x000055555555f116 in testCompareXMLToArgv (data=0x55555579f800 <info>) at qemuxml2argvtest.c:518 #4 0x0000555555584c50 in virTestRun (title=0x555555588e28 "QEMU XML-2-ARGV genid.x86_64-latest", body=0x55555555ecc3 <testCompareXMLToArgv>, data=0x55555579f800 <info>) at testutils.c:180 #5 0x000055555555fe52 in mymain () at qemuxml2argvtest.c:802 #6 0x0000555555586d0c in virTestMain (argc=1, argv=0x7fffffffd698, func=0x55555555f4f9 <mymain>) at testutils.c:1120 #7 0x00005555555832fe in main (argc=1, argv=0x7fffffffd698) at qemuxml2argvtest.c:2883 So testUpdateQEMUCaps() should free cpuData before calling virQEMUCapsInitHostCPUModel(). I'll prepare a patch that does that. Michal
participants (3)
-
Jiri Denemark -
Michal Privoznik -
Peter Krempa