[v1] vmx: Add support for NVRAM configuration

[PATCH] vmx: Add support for NVRAM configuration

Posted by Surya Gupta via Devel 3 weeks, 1 day ago

Some VMware guests specify NVRAM storage using the 'nvram' parameter.
If found, parse it and store it in the domain's os.loader.nvram field,
which gets formatted as:

  <os>
    <type arch='x86_64'>hvm</type>
    <nvram>[datastore] directory/dokuwiki.nvram</nvram>
  </os>

The NVRAM path uses the same transformation functions as disk paths
(ctx->parseFileName and ctx->formatFileName) to ensure consistent
handling of datastore-qualified paths.
The NVRAM is stored as a virStorageSource with type VIR_STORAGE_TYPE_FILE
to ensure compatibility with libvirt's existing firmware handling
infrastructure.

Fixes: https://redhat.atlassian.net/browse/RHELMISC-29421
Signed-off-by: Surya Gupta <surygupt@redhat.com>
---
 src/vmx/vmx.c                            | 32 ++++++++++++++++++++++++
 tests/vmx2xmldata/case-insensitive-1.xml |  1 +
 tests/vmx2xmldata/case-insensitive-2.xml |  1 +
 tests/vmx2xmldata/esx-in-the-wild-1.xml  |  1 +
 tests/vmx2xmldata/esx-in-the-wild-10.xml |  1 +
 tests/vmx2xmldata/esx-in-the-wild-11.xml |  1 +
 tests/vmx2xmldata/esx-in-the-wild-12.xml |  1 +
 tests/vmx2xmldata/esx-in-the-wild-13.xml |  1 +
 tests/vmx2xmldata/esx-in-the-wild-14.xml |  1 +
 tests/vmx2xmldata/esx-in-the-wild-15.xml |  1 +
 tests/vmx2xmldata/esx-in-the-wild-16.xml |  1 +
 tests/vmx2xmldata/esx-in-the-wild-17.xml |  1 +
 tests/vmx2xmldata/esx-in-the-wild-2.xml  |  1 +
 tests/vmx2xmldata/esx-in-the-wild-3.xml  |  1 +
 tests/vmx2xmldata/esx-in-the-wild-4.xml  |  1 +
 tests/vmx2xmldata/esx-in-the-wild-5.xml  |  1 +
 tests/vmx2xmldata/esx-in-the-wild-6.xml  |  1 +
 tests/vmx2xmldata/esx-in-the-wild-7.xml  |  1 +
 tests/vmx2xmldata/esx-in-the-wild-8.xml  |  1 +
 tests/vmx2xmldata/esx-in-the-wild-9.xml  |  1 +
 tests/vmx2xmldata/gsx-in-the-wild-1.xml  |  1 +
 tests/vmx2xmldata/gsx-in-the-wild-2.xml  |  1 +
 tests/vmx2xmldata/gsx-in-the-wild-3.xml  |  1 +
 tests/vmx2xmldata/gsx-in-the-wild-4.xml  |  1 +
 24 files changed, 55 insertions(+)

diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
index 57dfd57cfc..c42b030dd6 100644
--- a/src/vmx/vmx.c
+++ b/src/vmx/vmx.c
@@ -1415,6 +1415,7 @@ virVMXParseConfig(virVMXContext *ctx,
     long long coresPerSocket = 0;
     virCPUDef *cpu = NULL;
     char *firmware = NULL;
+    char *nvram = NULL;
     size_t saved_ndisks = 0;
 
     if (ctx->parseFileName == NULL) {
@@ -2022,6 +2023,26 @@ virVMXParseConfig(virVMXContext *ctx,
             VIR_TRISTATE_BOOL_YES;
     }
 
+    /* vmx:nvram */
+    if (virVMXGetConfigString(conf, "nvram", &nvram, true) < 0) {
+        goto cleanup;
+    }
+
+    if (nvram != NULL) {
+        g_autoptr(virStorageSource) n = virStorageSourceNew();
+        char *tmp = NULL;
+
+        if (!def->os.loader) {
+            def->os.loader = virDomainLoaderDefNew();
+        }
+
+        n->type = VIR_STORAGE_TYPE_FILE;
+        if (ctx->parseFileName(nvram, ctx->opaque, &tmp, false) < 0)
+            goto cleanup;
+        n->path = tmp;
+        def->os.loader->nvram = g_steal_pointer(&n);
+    }
+
     if (virDomainDefPostParse(def, VIR_DOMAIN_DEF_PARSE_ABI_UPDATE,
                               xmlopt, NULL) < 0)
         goto cleanup;
@@ -2035,6 +2056,7 @@ virVMXParseConfig(virVMXContext *ctx,
     VIR_FREE(guestOS);
     virCPUDefFree(cpu);
     VIR_FREE(firmware);
+    VIR_FREE(nvram);
 
     if (!success)
         return NULL;
@@ -3738,6 +3760,16 @@ virVMXFormatConfig(virVMXContext *ctx, virDomainXMLOption *xmlopt, virDomainDef
     if (def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_EFI)
         virBufferAddLit(&buffer, "firmware = \"efi\"\n");
 
+    /* vmx:nvram */
+    if (def->os.loader && def->os.loader->nvram && def->os.loader->nvram->path) {
+        g_autofree char *nvramPath = NULL;
+
+        nvramPath = ctx->formatFileName(def->os.loader->nvram->path, ctx->opaque);
+        if (nvramPath != NULL) {
+            virBufferAsprintf(&buffer, "nvram = \"%s\"\n", nvramPath);
+        }
+    }
+
     if (virtualHW_version >= 7) {
         if (hasSCSI) {
             virBufferAddLit(&buffer, "pciBridge0.present = \"true\"\n");
diff --git a/tests/vmx2xmldata/case-insensitive-1.xml b/tests/vmx2xmldata/case-insensitive-1.xml
index 7cb6413941..e854cc37cb 100644
--- a/tests/vmx2xmldata/case-insensitive-1.xml
+++ b/tests/vmx2xmldata/case-insensitive-1.xml
@@ -9,6 +9,7 @@
   </cputune>
   <os>
     <type arch='i686'>hvm</type>
+    <nvram>[datastore] directory/FEDORA11.NVRAM</nvram>
   </os>
   <clock offset='utc'/>
   <on_poweroff>destroy</on_poweroff>
diff --git a/tests/vmx2xmldata/case-insensitive-2.xml b/tests/vmx2xmldata/case-insensitive-2.xml
index 188c3f3cd5..f5c4446ab5 100644
--- a/tests/vmx2xmldata/case-insensitive-2.xml
+++ b/tests/vmx2xmldata/case-insensitive-2.xml
@@ -9,6 +9,7 @@
   </cputune>
   <os>
     <type arch='i686'>hvm</type>
+    <nvram>[datastore] directory/fedora11.nvram</nvram>
   </os>
   <clock offset='utc'/>
   <on_poweroff>destroy</on_poweroff>
diff --git a/tests/vmx2xmldata/esx-in-the-wild-1.xml b/tests/vmx2xmldata/esx-in-the-wild-1.xml
index c15275ccb9..9ae28c8497 100644
--- a/tests/vmx2xmldata/esx-in-the-wild-1.xml
+++ b/tests/vmx2xmldata/esx-in-the-wild-1.xml
@@ -9,6 +9,7 @@
   </cputune>
   <os>
     <type arch='i686'>hvm</type>
+    <nvram>[datastore] directory/Fedora11.nvram</nvram>
   </os>
   <clock offset='utc'/>
   <on_poweroff>destroy</on_poweroff>
diff --git a/tests/vmx2xmldata/esx-in-the-wild-10.xml b/tests/vmx2xmldata/esx-in-the-wild-10.xml
index 78129682bd..1b1fdf0662 100644
--- a/tests/vmx2xmldata/esx-in-the-wild-10.xml
+++ b/tests/vmx2xmldata/esx-in-the-wild-10.xml
@@ -10,6 +10,7 @@
   </cputune>
   <os>
     <type arch='x86_64'>hvm</type>
+    <nvram>[datastore] directory/windows2019biosvmware.nvram</nvram>
   </os>
   <cpu>
     <topology sockets='1' dies='1' clusters='1' cores='2' threads='1'/>
diff --git a/tests/vmx2xmldata/esx-in-the-wild-11.xml b/tests/vmx2xmldata/esx-in-the-wild-11.xml
index 8807a057d7..a0c1e05e90 100644
--- a/tests/vmx2xmldata/esx-in-the-wild-11.xml
+++ b/tests/vmx2xmldata/esx-in-the-wild-11.xml
@@ -9,6 +9,7 @@
   </cputune>
   <os>
     <type arch='x86_64'>hvm</type>
+    <nvram>[datastore] directory/esx6.7-rhel7.7-x86_64.nvram</nvram>
   </os>
   <clock offset='utc'/>
   <on_poweroff>destroy</on_poweroff>
diff --git a/tests/vmx2xmldata/esx-in-the-wild-12.xml b/tests/vmx2xmldata/esx-in-the-wild-12.xml
index c5aad90677..ac83982b9b 100644
--- a/tests/vmx2xmldata/esx-in-the-wild-12.xml
+++ b/tests/vmx2xmldata/esx-in-the-wild-12.xml
@@ -13,6 +13,7 @@
       <feature enabled='yes' name='enrolled-keys'/>
       <feature enabled='yes' name='secure-boot'/>
     </firmware>
+    <nvram>[datastore] directory/Auto-esx8.0-rhell9.3-efi-with-empty-cdrom.nvram</nvram>
   </os>
   <clock offset='utc'/>
   <on_poweroff>destroy</on_poweroff>
diff --git a/tests/vmx2xmldata/esx-in-the-wild-13.xml b/tests/vmx2xmldata/esx-in-the-wild-13.xml
index e6ef947d50..cef9fd4e48 100644
--- a/tests/vmx2xmldata/esx-in-the-wild-13.xml
+++ b/tests/vmx2xmldata/esx-in-the-wild-13.xml
@@ -23,6 +23,7 @@ package:20.6.2
   </cputune>
   <os>
     <type arch='x86_64'>hvm</type>
+    <nvram>[datastore] directory/Test-Mig-VM-1 (01ce57d0-4e20-41a5-8b6c-bcbf49a032ec).nvram</nvram>
   </os>
   <clock offset='utc'/>
   <on_poweroff>destroy</on_poweroff>
diff --git a/tests/vmx2xmldata/esx-in-the-wild-14.xml b/tests/vmx2xmldata/esx-in-the-wild-14.xml
index dd5c2434ee..f10707d1d4 100644
--- a/tests/vmx2xmldata/esx-in-the-wild-14.xml
+++ b/tests/vmx2xmldata/esx-in-the-wild-14.xml
@@ -7,6 +7,7 @@
   <vcpu placement='static'>12</vcpu>
   <os>
     <type arch='x86_64'>hvm</type>
+    <nvram>[datastore] directory/wild14.nvram</nvram>
   </os>
   <clock offset='utc'/>
   <on_poweroff>destroy</on_poweroff>
diff --git a/tests/vmx2xmldata/esx-in-the-wild-15.xml b/tests/vmx2xmldata/esx-in-the-wild-15.xml
index 77b094e9d5..78d15e1538 100644
--- a/tests/vmx2xmldata/esx-in-the-wild-15.xml
+++ b/tests/vmx2xmldata/esx-in-the-wild-15.xml
@@ -9,6 +9,7 @@
   </cputune>
   <os>
     <type arch='x86_64'>hvm</type>
+    <nvram>[datastore] directory/dokuwiki.nvram</nvram>
   </os>
   <clock offset='utc'/>
   <on_poweroff>destroy</on_poweroff>
diff --git a/tests/vmx2xmldata/esx-in-the-wild-16.xml b/tests/vmx2xmldata/esx-in-the-wild-16.xml
index 147bc0825a..51746dd77e 100644
--- a/tests/vmx2xmldata/esx-in-the-wild-16.xml
+++ b/tests/vmx2xmldata/esx-in-the-wild-16.xml
@@ -13,6 +13,7 @@
       <feature enabled='yes' name='enrolled-keys'/>
       <feature enabled='yes' name='secure-boot'/>
     </firmware>
+    <nvram>[datastore] directory/Auto-esx8.0-rhel9.4-efi-nvme-disk.nvram</nvram>
   </os>
   <clock offset='utc'/>
   <on_poweroff>destroy</on_poweroff>
diff --git a/tests/vmx2xmldata/esx-in-the-wild-17.xml b/tests/vmx2xmldata/esx-in-the-wild-17.xml
index ae66de7431..725f21bdf6 100644
--- a/tests/vmx2xmldata/esx-in-the-wild-17.xml
+++ b/tests/vmx2xmldata/esx-in-the-wild-17.xml
@@ -10,6 +10,7 @@
   </cputune>
   <os>
     <type arch='x86_64'>hvm</type>
+    <nvram>[datastore] directory/esx8.0-win11-with-second-disk-in-subfolder.nvram</nvram>
   </os>
   <clock offset='utc'/>
   <on_poweroff>destroy</on_poweroff>
diff --git a/tests/vmx2xmldata/esx-in-the-wild-2.xml b/tests/vmx2xmldata/esx-in-the-wild-2.xml
index 59071b5d3a..59c7087300 100644
--- a/tests/vmx2xmldata/esx-in-the-wild-2.xml
+++ b/tests/vmx2xmldata/esx-in-the-wild-2.xml
@@ -6,6 +6,7 @@
   <vcpu placement='static' cpuset='0-2,5-7'>4</vcpu>
   <os>
     <type arch='x86_64'>hvm</type>
+    <nvram>[datastore] directory/Debian1.nvram</nvram>
   </os>
   <clock offset='utc'/>
   <on_poweroff>destroy</on_poweroff>
diff --git a/tests/vmx2xmldata/esx-in-the-wild-3.xml b/tests/vmx2xmldata/esx-in-the-wild-3.xml
index cbe8eceb37..29c63d8d6b 100644
--- a/tests/vmx2xmldata/esx-in-the-wild-3.xml
+++ b/tests/vmx2xmldata/esx-in-the-wild-3.xml
@@ -6,6 +6,7 @@
   <vcpu placement='static' cpuset='0,3-5'>2</vcpu>
   <os>
     <type arch='x86_64'>hvm</type>
+    <nvram>[datastore] directory/Debian2.nvram</nvram>
   </os>
   <clock offset='utc'/>
   <on_poweroff>destroy</on_poweroff>
diff --git a/tests/vmx2xmldata/esx-in-the-wild-4.xml b/tests/vmx2xmldata/esx-in-the-wild-4.xml
index a8a2ac6f97..82eccca1c4 100644
--- a/tests/vmx2xmldata/esx-in-the-wild-4.xml
+++ b/tests/vmx2xmldata/esx-in-the-wild-4.xml
@@ -9,6 +9,7 @@
   </cputune>
   <os>
     <type arch='i686'>hvm</type>
+    <nvram>[datastore] directory/virtMonServ1.nvram</nvram>
   </os>
   <clock offset='utc'/>
   <on_poweroff>destroy</on_poweroff>
diff --git a/tests/vmx2xmldata/esx-in-the-wild-5.xml b/tests/vmx2xmldata/esx-in-the-wild-5.xml
index 9eb975afe9..c88e60bdc0 100644
--- a/tests/vmx2xmldata/esx-in-the-wild-5.xml
+++ b/tests/vmx2xmldata/esx-in-the-wild-5.xml
@@ -13,6 +13,7 @@
   </cputune>
   <os>
     <type arch='x86_64'>hvm</type>
+    <nvram>[datastore] directory/vmtest.nvram</nvram>
   </os>
   <clock offset='utc'/>
   <on_poweroff>destroy</on_poweroff>
diff --git a/tests/vmx2xmldata/esx-in-the-wild-6.xml b/tests/vmx2xmldata/esx-in-the-wild-6.xml
index 51c74dd8a1..805f033561 100644
--- a/tests/vmx2xmldata/esx-in-the-wild-6.xml
+++ b/tests/vmx2xmldata/esx-in-the-wild-6.xml
@@ -6,6 +6,7 @@
   <vcpu placement='static'>1</vcpu>
   <os>
     <type arch='x86_64'>hvm</type>
+    <nvram>[datastore] directory/el6-test.nvram</nvram>
   </os>
   <clock offset='utc'/>
   <on_poweroff>destroy</on_poweroff>
diff --git a/tests/vmx2xmldata/esx-in-the-wild-7.xml b/tests/vmx2xmldata/esx-in-the-wild-7.xml
index c117bd62e5..b641574776 100644
--- a/tests/vmx2xmldata/esx-in-the-wild-7.xml
+++ b/tests/vmx2xmldata/esx-in-the-wild-7.xml
@@ -6,6 +6,7 @@
   <vcpu placement='static'>1</vcpu>
   <os>
     <type arch='x86_64'>hvm</type>
+    <nvram>[datastore] directory/esx-rhel6-mini.nvram</nvram>
   </os>
   <clock offset='utc'/>
   <on_poweroff>destroy</on_poweroff>
diff --git a/tests/vmx2xmldata/esx-in-the-wild-8.xml b/tests/vmx2xmldata/esx-in-the-wild-8.xml
index 47d22ced2a..f13e6f7448 100644
--- a/tests/vmx2xmldata/esx-in-the-wild-8.xml
+++ b/tests/vmx2xmldata/esx-in-the-wild-8.xml
@@ -9,6 +9,7 @@
   </cputune>
   <os>
     <type arch='x86_64'>hvm</type>
+    <nvram>[datastore] directory/RHEL7_6.nvram</nvram>
   </os>
   <cpu>
     <topology sockets='4' dies='1' clusters='1' cores='2' threads='1'/>
diff --git a/tests/vmx2xmldata/esx-in-the-wild-9.xml b/tests/vmx2xmldata/esx-in-the-wild-9.xml
index ee6be2527f..6b4d878ab1 100644
--- a/tests/vmx2xmldata/esx-in-the-wild-9.xml
+++ b/tests/vmx2xmldata/esx-in-the-wild-9.xml
@@ -10,6 +10,7 @@
   </cputune>
   <os>
     <type arch='x86_64'>hvm</type>
+    <nvram>[datastore] directory/v2v-windows-kkulkarn.nvram</nvram>
   </os>
   <cpu>
     <topology sockets='4' dies='1' clusters='1' cores='4' threads='1'/>
diff --git a/tests/vmx2xmldata/gsx-in-the-wild-1.xml b/tests/vmx2xmldata/gsx-in-the-wild-1.xml
index 62ec191c82..f189ff79e4 100644
--- a/tests/vmx2xmldata/gsx-in-the-wild-1.xml
+++ b/tests/vmx2xmldata/gsx-in-the-wild-1.xml
@@ -6,6 +6,7 @@
   <vcpu placement='static'>1</vcpu>
   <os>
     <type arch='i686'>hvm</type>
+    <nvram>[datastore] directory/Debian-System1.nvram</nvram>
   </os>
   <clock offset='utc'/>
   <on_poweroff>destroy</on_poweroff>
diff --git a/tests/vmx2xmldata/gsx-in-the-wild-2.xml b/tests/vmx2xmldata/gsx-in-the-wild-2.xml
index 906e4657ca..d1c1bf39df 100644
--- a/tests/vmx2xmldata/gsx-in-the-wild-2.xml
+++ b/tests/vmx2xmldata/gsx-in-the-wild-2.xml
@@ -6,6 +6,7 @@
   <vcpu placement='static'>1</vcpu>
   <os>
     <type arch='i686'>hvm</type>
+    <nvram>[datastore] directory/Server2.nvram</nvram>
   </os>
   <clock offset='utc'/>
   <on_poweroff>destroy</on_poweroff>
diff --git a/tests/vmx2xmldata/gsx-in-the-wild-3.xml b/tests/vmx2xmldata/gsx-in-the-wild-3.xml
index 61812851e1..acc9d6ba5d 100644
--- a/tests/vmx2xmldata/gsx-in-the-wild-3.xml
+++ b/tests/vmx2xmldata/gsx-in-the-wild-3.xml
@@ -6,6 +6,7 @@
   <vcpu placement='static'>1</vcpu>
   <os>
     <type arch='i686'>hvm</type>
+    <nvram>[datastore] directory/Debian-System3.nvram</nvram>
   </os>
   <clock offset='utc'/>
   <on_poweroff>destroy</on_poweroff>
diff --git a/tests/vmx2xmldata/gsx-in-the-wild-4.xml b/tests/vmx2xmldata/gsx-in-the-wild-4.xml
index a65a7d137f..8c73224846 100644
--- a/tests/vmx2xmldata/gsx-in-the-wild-4.xml
+++ b/tests/vmx2xmldata/gsx-in-the-wild-4.xml
@@ -6,6 +6,7 @@
   <vcpu placement='static'>1</vcpu>
   <os>
     <type arch='i686'>hvm</type>
+    <nvram>[datastore] directory/Debian-System4.nvram</nvram>
   </os>
   <clock offset='utc'/>
   <on_poweroff>destroy</on_poweroff>
-- 
2.53.0

Re: [PATCH] vmx: Add support for NVRAM configuration

Posted by Peter Krempa via Devel 3 weeks, 1 day ago

On Tue, Apr 21, 2026 at 11:00:21 +0530, Surya Gupta via Devel wrote:
> Some VMware guests specify NVRAM storage using the 'nvram' parameter.
> If found, parse it and store it in the domain's os.loader.nvram field,
> which gets formatted as:
> 
>   <os>
>     <type arch='x86_64'>hvm</type>
>     <nvram>[datastore] directory/dokuwiki.nvram</nvram>
>   </os>
> 
> The NVRAM path uses the same transformation functions as disk paths
> (ctx->parseFileName and ctx->formatFileName) to ensure consistent
> handling of datastore-qualified paths.
> The NVRAM is stored as a virStorageSource with type VIR_STORAGE_TYPE_FILE
> to ensure compatibility with libvirt's existing firmware handling
> infrastructure.
> 
> Fixes: https://redhat.atlassian.net/browse/RHELMISC-29421

This issue isn't public (insn't accessible without being logged in). Do
not mention it in public commit messages.


> Signed-off-by: Surya Gupta <surygupt@redhat.com>
> ---
>  src/vmx/vmx.c                            | 32 ++++++++++++++++++++++++
>  tests/vmx2xmldata/case-insensitive-1.xml |  1 +
>  tests/vmx2xmldata/case-insensitive-2.xml |  1 +
>  tests/vmx2xmldata/esx-in-the-wild-1.xml  |  1 +
>  tests/vmx2xmldata/esx-in-the-wild-10.xml |  1 +
>  tests/vmx2xmldata/esx-in-the-wild-11.xml |  1 +
>  tests/vmx2xmldata/esx-in-the-wild-12.xml |  1 +
>  tests/vmx2xmldata/esx-in-the-wild-13.xml |  1 +
>  tests/vmx2xmldata/esx-in-the-wild-14.xml |  1 +
>  tests/vmx2xmldata/esx-in-the-wild-15.xml |  1 +
>  tests/vmx2xmldata/esx-in-the-wild-16.xml |  1 +
>  tests/vmx2xmldata/esx-in-the-wild-17.xml |  1 +
>  tests/vmx2xmldata/esx-in-the-wild-2.xml  |  1 +
>  tests/vmx2xmldata/esx-in-the-wild-3.xml  |  1 +
>  tests/vmx2xmldata/esx-in-the-wild-4.xml  |  1 +
>  tests/vmx2xmldata/esx-in-the-wild-5.xml  |  1 +
>  tests/vmx2xmldata/esx-in-the-wild-6.xml  |  1 +
>  tests/vmx2xmldata/esx-in-the-wild-7.xml  |  1 +
>  tests/vmx2xmldata/esx-in-the-wild-8.xml  |  1 +
>  tests/vmx2xmldata/esx-in-the-wild-9.xml  |  1 +
>  tests/vmx2xmldata/gsx-in-the-wild-1.xml  |  1 +
>  tests/vmx2xmldata/gsx-in-the-wild-2.xml  |  1 +
>  tests/vmx2xmldata/gsx-in-the-wild-3.xml  |  1 +
>  tests/vmx2xmldata/gsx-in-the-wild-4.xml  |  1 +
>  24 files changed, 55 insertions(+)
> 
> diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
> index 57dfd57cfc..c42b030dd6 100644
> --- a/src/vmx/vmx.c
> +++ b/src/vmx/vmx.c
> @@ -1415,6 +1415,7 @@ virVMXParseConfig(virVMXContext *ctx,
>      long long coresPerSocket = 0;
>      virCPUDef *cpu = NULL;
>      char *firmware = NULL;
> +    char *nvram = NULL;

[1]

>      size_t saved_ndisks = 0;
>  
>      if (ctx->parseFileName == NULL) {
> @@ -2022,6 +2023,26 @@ virVMXParseConfig(virVMXContext *ctx,
>              VIR_TRISTATE_BOOL_YES;
>      }
>  
> +    /* vmx:nvram */
> +    if (virVMXGetConfigString(conf, "nvram", &nvram, true) < 0) {
> +        goto cleanup;
> +    }
> +
> +    if (nvram != NULL) {
> +        g_autoptr(virStorageSource) n = virStorageSourceNew();

You use 'autoptr' here, why not declare the 'nvram' string [1] as:

  g_autofree char *nvram = NULL ?

> +        char *tmp = NULL;
> +
> +        if (!def->os.loader) {
> +            def->os.loader = virDomainLoaderDefNew();

Can this ever be already allocated? [2]

> +        }
> +
> +        n->type = VIR_STORAGE_TYPE_FILE;
> +        if (ctx->parseFileName(nvram, ctx->opaque, &tmp, false) < 0)

n->path is also a 'char *' you can use it instead of the extra 'tmp'
variable ...

> +            goto cleanup;
> +        n->path = tmp;

... if you otherwise directly assign it.

> +        def->os.loader->nvram = g_steal_pointer(&n);

[2] I'm asking because this then could overwrite existing value here
leaking the original.

> +    }
> +
>      if (virDomainDefPostParse(def, VIR_DOMAIN_DEF_PARSE_ABI_UPDATE,
>                                xmlopt, NULL) < 0)
>          goto cleanup;
> @@ -2035,6 +2056,7 @@ virVMXParseConfig(virVMXContext *ctx,
>      VIR_FREE(guestOS);
>      virCPUDefFree(cpu);
>      VIR_FREE(firmware);
> +    VIR_FREE(nvram);

this can be skipped if autofree is used.

Re: [PATCH] vmx: Add support for NVRAM configuration

Posted by surygupt--- via Devel 3 weeks ago

Peter Krempa wrote:
> > +        if (!def->os.loader) {
> > +            def->os.loader = virDomainLoaderDefNew();
> 
> Can this ever be already allocated? [2]
>
> [2] I'm asking because this then could overwrite existing value here
> leaking the original.

I checked virVMXParseConfig(), it creates a fresh virDomainDef at line 1460
so the nil check can be removed and simplified to:

    def->os.loader = virDomainLoaderDefNew();

I have fixed other changes as you suggested, do let me know if above looks good.
Will send v2 shortly with all fixes applied.

Thanks for the review!

Re: [PATCH] vmx: Add support for NVRAM configuration

Posted by Peter Krempa via Devel 2 weeks, 5 days ago

On Tue, Apr 21, 2026 at 19:36:25 -0000, surygupt--- via Devel wrote:
> Peter Krempa wrote:
> > > +        if (!def->os.loader) {
> > > +            def->os.loader = virDomainLoaderDefNew();
> > 
> > Can this ever be already allocated? [2]
> >
> > [2] I'm asking because this then could overwrite existing value here
> > leaking the original.
> 
> I checked virVMXParseConfig(), it creates a fresh virDomainDef at line 1460
> so the nil check can be removed and simplified to:
> 
>     def->os.loader = virDomainLoaderDefNew();
> 
> I have fixed other changes as you suggested, do let me know if above looks good.
> Will send v2 shortly with all fixes applied.

Yes this makes sense.