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.

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.

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?