[PATCH] qemu: Don't regenerate NVRAM path if parsed from domain XML
After v8.0.0-466-g08101bde5d we unconditionally regenerate per domain NVRAM path even though it might have been parsed earlier from domain XML. The way we do that leads to a memleak: 43 bytes in 1 blocks are definitely lost in loss record 330 of 682 at 0x483F7E5: malloc (vg_replace_malloc.c:381) by 0x50D5B18: g_malloc (in /usr/lib64/libglib-2.0.so.0.7000.2) by 0x50EFA4F: g_strdup (in /usr/lib64/libglib-2.0.so.0.7000.2) by 0x49E774E: virXPathString (virxml.c:88) by 0x4A3F0E4: virDomainDefParseBootLoaderOptions (domain_conf.c:18226) by 0x4A3F49C: virDomainDefParseBootOptions (domain_conf.c:18298) by 0x4A448C3: virDomainDefParseXML (domain_conf.c:19598) by 0x4A487A1: virDomainDefParseNode (domain_conf.c:20404) by 0x117FCF: testCompareXMLToArgv (qemuxml2argvtest.c:726) by 0x142124: virTestRun (testutils.c:142) by 0x1423D4: virTestRunLog (testutils.c:197) by 0x140A76: mymain (qemuxml2argvtest.c:3406) If we parsed NVRAM path from domain XML we must refrain from generating new path. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_firmware.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c index 4b02cb2802..59dc76fec4 100644 --- a/src/qemu/qemu_firmware.c +++ b/src/qemu/qemu_firmware.c @@ -1139,7 +1139,8 @@ qemuFirmwareEnableFeatures(virQEMUDriver *driver, VIR_FREE(def->os.loader->nvramTemplate); def->os.loader->nvramTemplate = g_strdup(flash->nvram_template.filename); - qemuDomainNVRAMPathFormat(cfg, def, &def->os.loader->nvram); + if (!def->os.loader->nvram) + qemuDomainNVRAMPathFormat(cfg, def, &def->os.loader->nvram); VIR_DEBUG("decided on firmware '%s' template '%s' NVRAM '%s'", def->os.loader->path, -- 2.34.1
On Wed, Feb 23, 2022 at 10:16:57 +0100, Michal Privoznik wrote:
After v8.0.0-466-g08101bde5d we unconditionally regenerate per domain NVRAM path even though it might have been parsed earlier from domain XML. The way we do that leads to a memleak:
43 bytes in 1 blocks are definitely lost in loss record 330 of 682 at 0x483F7E5: malloc (vg_replace_malloc.c:381) by 0x50D5B18: g_malloc (in /usr/lib64/libglib-2.0.so.0.7000.2) by 0x50EFA4F: g_strdup (in /usr/lib64/libglib-2.0.so.0.7000.2) by 0x49E774E: virXPathString (virxml.c:88) by 0x4A3F0E4: virDomainDefParseBootLoaderOptions (domain_conf.c:18226) by 0x4A3F49C: virDomainDefParseBootOptions (domain_conf.c:18298) by 0x4A448C3: virDomainDefParseXML (domain_conf.c:19598) by 0x4A487A1: virDomainDefParseNode (domain_conf.c:20404) by 0x117FCF: testCompareXMLToArgv (qemuxml2argvtest.c:726) by 0x142124: virTestRun (testutils.c:142) by 0x1423D4: virTestRunLog (testutils.c:197) by 0x140A76: mymain (qemuxml2argvtest.c:3406)
If we parsed NVRAM path from domain XML we must refrain from generating new path.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_firmware.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
On Wed, Feb 23, 2022 at 10:16:57AM +0100, Michal Privoznik wrote:
After v8.0.0-466-g08101bde5d we unconditionally regenerate per domain NVRAM path even though it might have been parsed earlier from domain XML. The way we do that leads to a memleak:
43 bytes in 1 blocks are definitely lost in loss record 330 of 682 at 0x483F7E5: malloc (vg_replace_malloc.c:381) by 0x50D5B18: g_malloc (in /usr/lib64/libglib-2.0.so.0.7000.2) by 0x50EFA4F: g_strdup (in /usr/lib64/libglib-2.0.so.0.7000.2) by 0x49E774E: virXPathString (virxml.c:88) by 0x4A3F0E4: virDomainDefParseBootLoaderOptions (domain_conf.c:18226) by 0x4A3F49C: virDomainDefParseBootOptions (domain_conf.c:18298) by 0x4A448C3: virDomainDefParseXML (domain_conf.c:19598) by 0x4A487A1: virDomainDefParseNode (domain_conf.c:20404) by 0x117FCF: testCompareXMLToArgv (qemuxml2argvtest.c:726) by 0x142124: virTestRun (testutils.c:142) by 0x1423D4: virTestRunLog (testutils.c:197) by 0x140A76: mymain (qemuxml2argvtest.c:3406)
If we parsed NVRAM path from domain XML we must refrain from generating new path.
Hmm, so we honour the 'nvram' path from the XML, even when doing firmware auto-select ? That is contrary to the qemuDomainUndefineFlags method expectations which unconditionally uses the qemuDomainNVRAMPathFormat result when deleting nvram, ignoring 'nvram' path in the XML Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 2/23/22 10:33, Daniel P. Berrangé wrote:
On Wed, Feb 23, 2022 at 10:16:57AM +0100, Michal Privoznik wrote:
After v8.0.0-466-g08101bde5d we unconditionally regenerate per domain NVRAM path even though it might have been parsed earlier from domain XML. The way we do that leads to a memleak:
43 bytes in 1 blocks are definitely lost in loss record 330 of 682 at 0x483F7E5: malloc (vg_replace_malloc.c:381) by 0x50D5B18: g_malloc (in /usr/lib64/libglib-2.0.so.0.7000.2) by 0x50EFA4F: g_strdup (in /usr/lib64/libglib-2.0.so.0.7000.2) by 0x49E774E: virXPathString (virxml.c:88) by 0x4A3F0E4: virDomainDefParseBootLoaderOptions (domain_conf.c:18226) by 0x4A3F49C: virDomainDefParseBootOptions (domain_conf.c:18298) by 0x4A448C3: virDomainDefParseXML (domain_conf.c:19598) by 0x4A487A1: virDomainDefParseNode (domain_conf.c:20404) by 0x117FCF: testCompareXMLToArgv (qemuxml2argvtest.c:726) by 0x142124: virTestRun (testutils.c:142) by 0x1423D4: virTestRunLog (testutils.c:197) by 0x140A76: mymain (qemuxml2argvtest.c:3406)
If we parsed NVRAM path from domain XML we must refrain from generating new path.
Hmm, so we honour the 'nvram' path from the XML, even when doing firmware auto-select ?
That is contrary to the qemuDomainUndefineFlags method expectations which unconditionally uses the qemuDomainNVRAMPathFormat result when deleting nvram, ignoring 'nvram' path in the XML
Good point. I think the question boils down to, what should happen when FW autoselection is enabled and user told use where they want to have NVRAM stored? It's a tricky situation because if the NVRAM file does exist we won't overwrite it. And yet, if we ever selected a different FW image the pre-existing NVRAM might be incompatible with the new FW image. Michal
On Wed, Feb 23, 2022 at 10:47:20AM +0100, Michal Prívozník wrote:
On 2/23/22 10:33, Daniel P. Berrangé wrote:
On Wed, Feb 23, 2022 at 10:16:57AM +0100, Michal Privoznik wrote:
After v8.0.0-466-g08101bde5d we unconditionally regenerate per domain NVRAM path even though it might have been parsed earlier from domain XML. The way we do that leads to a memleak:
43 bytes in 1 blocks are definitely lost in loss record 330 of 682 at 0x483F7E5: malloc (vg_replace_malloc.c:381) by 0x50D5B18: g_malloc (in /usr/lib64/libglib-2.0.so.0.7000.2) by 0x50EFA4F: g_strdup (in /usr/lib64/libglib-2.0.so.0.7000.2) by 0x49E774E: virXPathString (virxml.c:88) by 0x4A3F0E4: virDomainDefParseBootLoaderOptions (domain_conf.c:18226) by 0x4A3F49C: virDomainDefParseBootOptions (domain_conf.c:18298) by 0x4A448C3: virDomainDefParseXML (domain_conf.c:19598) by 0x4A487A1: virDomainDefParseNode (domain_conf.c:20404) by 0x117FCF: testCompareXMLToArgv (qemuxml2argvtest.c:726) by 0x142124: virTestRun (testutils.c:142) by 0x1423D4: virTestRunLog (testutils.c:197) by 0x140A76: mymain (qemuxml2argvtest.c:3406)
If we parsed NVRAM path from domain XML we must refrain from generating new path.
Hmm, so we honour the 'nvram' path from the XML, even when doing firmware auto-select ?
That is contrary to the qemuDomainUndefineFlags method expectations which unconditionally uses the qemuDomainNVRAMPathFormat result when deleting nvram, ignoring 'nvram' path in the XML
Good point. I think the question boils down to, what should happen when FW autoselection is enabled and user told use where they want to have NVRAM stored? It's a tricky situation because if the NVRAM file does exist we won't overwrite it. And yet, if we ever selected a different FW image the pre-existing NVRAM might be incompatible with the new FW image.
The new RESET_NVRAM flag can recover from this last scenario now. So we need to make the qemuDomainUndefineFlags method honour the nvram path, if it was provided. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 2/23/22 11:01, Daniel P. Berrangé wrote:
On Wed, Feb 23, 2022 at 10:47:20AM +0100, Michal Prívozník wrote:
On 2/23/22 10:33, Daniel P. Berrangé wrote:
On Wed, Feb 23, 2022 at 10:16:57AM +0100, Michal Privoznik wrote:
After v8.0.0-466-g08101bde5d we unconditionally regenerate per domain NVRAM path even though it might have been parsed earlier from domain XML. The way we do that leads to a memleak:
43 bytes in 1 blocks are definitely lost in loss record 330 of 682 at 0x483F7E5: malloc (vg_replace_malloc.c:381) by 0x50D5B18: g_malloc (in /usr/lib64/libglib-2.0.so.0.7000.2) by 0x50EFA4F: g_strdup (in /usr/lib64/libglib-2.0.so.0.7000.2) by 0x49E774E: virXPathString (virxml.c:88) by 0x4A3F0E4: virDomainDefParseBootLoaderOptions (domain_conf.c:18226) by 0x4A3F49C: virDomainDefParseBootOptions (domain_conf.c:18298) by 0x4A448C3: virDomainDefParseXML (domain_conf.c:19598) by 0x4A487A1: virDomainDefParseNode (domain_conf.c:20404) by 0x117FCF: testCompareXMLToArgv (qemuxml2argvtest.c:726) by 0x142124: virTestRun (testutils.c:142) by 0x1423D4: virTestRunLog (testutils.c:197) by 0x140A76: mymain (qemuxml2argvtest.c:3406)
If we parsed NVRAM path from domain XML we must refrain from generating new path.
Hmm, so we honour the 'nvram' path from the XML, even when doing firmware auto-select ?
That is contrary to the qemuDomainUndefineFlags method expectations which unconditionally uses the qemuDomainNVRAMPathFormat result when deleting nvram, ignoring 'nvram' path in the XML
Good point. I think the question boils down to, what should happen when FW autoselection is enabled and user told use where they want to have NVRAM stored? It's a tricky situation because if the NVRAM file does exist we won't overwrite it. And yet, if we ever selected a different FW image the pre-existing NVRAM might be incompatible with the new FW image.
The new RESET_NVRAM flag can recover from this last scenario now.
So we need to make the qemuDomainUndefineFlags method honour the nvram path, if it was provided.
Fair enough. So do you prefer a follow up patch for fixing qemuDomainUndefineFlags() or reverting this patch and regenerating NVRAM path always? Michal
On Wed, Feb 23, 2022 at 12:47:29PM +0100, Michal Prívozník wrote:
On 2/23/22 11:01, Daniel P. Berrangé wrote:
On Wed, Feb 23, 2022 at 10:47:20AM +0100, Michal Prívozník wrote:
On 2/23/22 10:33, Daniel P. Berrangé wrote:
On Wed, Feb 23, 2022 at 10:16:57AM +0100, Michal Privoznik wrote:
After v8.0.0-466-g08101bde5d we unconditionally regenerate per domain NVRAM path even though it might have been parsed earlier from domain XML. The way we do that leads to a memleak:
43 bytes in 1 blocks are definitely lost in loss record 330 of 682 at 0x483F7E5: malloc (vg_replace_malloc.c:381) by 0x50D5B18: g_malloc (in /usr/lib64/libglib-2.0.so.0.7000.2) by 0x50EFA4F: g_strdup (in /usr/lib64/libglib-2.0.so.0.7000.2) by 0x49E774E: virXPathString (virxml.c:88) by 0x4A3F0E4: virDomainDefParseBootLoaderOptions (domain_conf.c:18226) by 0x4A3F49C: virDomainDefParseBootOptions (domain_conf.c:18298) by 0x4A448C3: virDomainDefParseXML (domain_conf.c:19598) by 0x4A487A1: virDomainDefParseNode (domain_conf.c:20404) by 0x117FCF: testCompareXMLToArgv (qemuxml2argvtest.c:726) by 0x142124: virTestRun (testutils.c:142) by 0x1423D4: virTestRunLog (testutils.c:197) by 0x140A76: mymain (qemuxml2argvtest.c:3406)
If we parsed NVRAM path from domain XML we must refrain from generating new path.
Hmm, so we honour the 'nvram' path from the XML, even when doing firmware auto-select ?
That is contrary to the qemuDomainUndefineFlags method expectations which unconditionally uses the qemuDomainNVRAMPathFormat result when deleting nvram, ignoring 'nvram' path in the XML
Good point. I think the question boils down to, what should happen when FW autoselection is enabled and user told use where they want to have NVRAM stored? It's a tricky situation because if the NVRAM file does exist we won't overwrite it. And yet, if we ever selected a different FW image the pre-existing NVRAM might be incompatible with the new FW image.
The new RESET_NVRAM flag can recover from this last scenario now.
So we need to make the qemuDomainUndefineFlags method honour the nvram path, if it was provided.
Fair enough. So do you prefer a follow up patch for fixing qemuDomainUndefineFlags() or reverting this patch and regenerating NVRAM path always?
I'll send a patch for qemuDomainUndefineFlags shortly. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
participants (4)
-
Daniel P. Berrangé -
Michal Privoznik -
Michal Prívozník
-
Peter Krempa