5:16 p.m.
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(a)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
5:24 p.m.
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(a)redhat.com>
---
src/qemu/qemu_firmware.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
5:33 p.m.
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 :|
5:47 p.m.
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
6:01 p.m.
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 :|
7:47 p.m.
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
8:21 p.m.
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 :|
981
days inactive
981
days old
6 comments
4 participants
participants (4)
-
Daniel P. Berrangé -
Michal Privoznik -
Michal Prívozník
-
Peter Krempa