1. Patchew
  2. Libvirt
  3. View series

[libvirt PATCH] conf: remove duplicated firmware type attribute

Daniel P. Berrangé posted 1 patch 3 years ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20210329180352.101829-1-berrange@redhat.com
docs/formatdomain.rst                         |  8 ----
docs/schemas/domaincommon.rng                 | 10 +---
src/conf/domain_conf.c                        | 48 ++++++-------------
.../os-firmware-efi-no-enrolled-keys.xml      |  2 +-
.../os-firmware-invalid-type.xml              | 28 -----------
tests/qemuxml2argvtest.c                      |  1 -
...aarch64-os-firmware-efi.aarch64-latest.xml |  1 -
.../os-firmware-bios.x86_64-latest.xml        |  1 -
.../os-firmware-efi-secboot.x86_64-latest.xml |  1 -
.../os-firmware-efi.x86_64-latest.xml         |  1 -
tests/vmx2xmldata/vmx2xml-firmware-efi.xml    |  1 -
11 files changed, 18 insertions(+), 84 deletions(-)
delete mode 100644 tests/qemuxml2argvdata/os-firmware-invalid-type.xml
[libvirt PATCH] conf: remove duplicated firmware type attribute
Posted by Daniel P. Berrangé 3 years ago 
The

  <os firmware='efi'>
    <firmware type='efi'>
      <feature enabled='no' name='enrolled-keys'/>
    </firmware>
  </os>

repeats the firmware attribute twice. This has no functional benefit, as
evidenced by fact that we use a single struct field to store both
attributes, while needlessly introducing an error scenario. The XML can
just be simplified to:

  <os firmware='efi'>
    <firmware>
      <feature enabled='no' name='enrolled-keys'/>
    </firmware>
  </os>

which also means that we don't need to emit the empty element
<firmware type='efi'/> for all existing configs too.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 docs/formatdomain.rst                         |  8 ----
 docs/schemas/domaincommon.rng                 | 10 +---
 src/conf/domain_conf.c                        | 48 ++++++-------------
 .../os-firmware-efi-no-enrolled-keys.xml      |  2 +-
 .../os-firmware-invalid-type.xml              | 28 -----------
 tests/qemuxml2argvtest.c                      |  1 -
 ...aarch64-os-firmware-efi.aarch64-latest.xml |  1 -
 .../os-firmware-bios.x86_64-latest.xml        |  1 -
 .../os-firmware-efi-secboot.x86_64-latest.xml |  1 -
 .../os-firmware-efi.x86_64-latest.xml         |  1 -
 tests/vmx2xmldata/vmx2xml-firmware-efi.xml    |  1 -
 11 files changed, 18 insertions(+), 84 deletions(-)
 delete mode 100644 tests/qemuxml2argvdata/os-firmware-invalid-type.xml

diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index 9392c80113..741130bf21 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -158,14 +158,6 @@ harddisk, cdrom, network) determining where to obtain/find the boot image.
 ``firmware``
    :since:`Since 7.2.0 QEMU/KVM only`
 
-   When used together with ``firmware`` attribute of ``os`` element the ``type``
-   attribute must have the same value.
-
-   List of mandatory attributes:
-
-   - ``type`` (accepted values are ``bios`` and ``efi``) same as the ``firmware``
-     attribute of ``os`` element.
-
    When using firmware auto-selection there are different features enabled in
    the firmwares. The list of features can be used to limit what firmware should
    be automatically selected for the VM. The list of features can be specified
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 1dbfc68f18..f5ced5b7a2 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -278,13 +278,7 @@
         <ref name="ostypehvm"/>
         <optional>
           <element name="firmware">
-            <attribute name="type">
-              <choice>
-                <value>bios</value>
-                <value>efi</value>
-              </choice>
-            </attribute>
-            <zeroOrMore>
+            <oneOrMore>
               <element name="feature">
                 <attribute name="enabled">
                   <ref name="virYesNo"/>
@@ -296,7 +290,7 @@
                   </choice>
                 </attribute>
               </element>
-            </zeroOrMore>
+            </oneOrMore>
           </element>
         </optional>
         <optional>
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index b0eba9f7bd..d050a519c6 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -19590,31 +19590,21 @@ virDomainDefParseBootFirmwareOptions(virDomainDefPtr def,
                                      xmlXPathContextPtr ctxt)
 {
     g_autofree char *firmware = virXPathString("string(./os/@firmware)", ctxt);
-    g_autofree char *type = virXPathString("string(./os/firmware/@type)", ctxt);
     g_autofree xmlNodePtr *nodes = NULL;
     g_autofree int *features = NULL;
     int fw = 0;
     int n = 0;
     size_t i;
 
-    if (!firmware && !type)
+    if (!firmware)
         return 0;
 
-    if (firmware && type && STRNEQ(firmware, type)) {
-        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-                       _("firmware attribute and firmware type has to be the same"));
-        return -1;
-    }
-
-    if (!type)
-        type = g_steal_pointer(&firmware);
-
-    fw = virDomainOsDefFirmwareTypeFromString(type);
+    fw = virDomainOsDefFirmwareTypeFromString(firmware);
 
     if (fw <= 0) {
         virReportError(VIR_ERR_XML_ERROR,
                        _("unknown firmware value %s"),
-                       type);
+                       firmware);
         return -1;
     }
 
@@ -29491,30 +29481,22 @@ virDomainDefFormatInternalSetRootName(virDomainDefPtr def,
         virBufferAsprintf(buf, ">%s</type>\n",
                           virDomainOSTypeToString(def->os.type));
 
-    if (def->os.firmware) {
-        virBufferAsprintf(buf, "<firmware type='%s'",
-                          virDomainOsDefFirmwareTypeToString(def->os.firmware));
-
-        if (def->os.firmwareFeatures) {
-            virBufferAddLit(buf, ">\n");
-
-            virBufferAdjustIndent(buf, 2);
+    if (def->os.firmwareFeatures) {
+        virBufferAddLit(buf, "<firmware>\n");
+        virBufferAdjustIndent(buf, 2);
 
-            for (i = 0; i < VIR_DOMAIN_OS_DEF_FIRMWARE_FEATURE_LAST; i++) {
-                if (def->os.firmwareFeatures[i] == VIR_TRISTATE_BOOL_ABSENT)
-                    continue;
+        for (i = 0; i < VIR_DOMAIN_OS_DEF_FIRMWARE_FEATURE_LAST; i++) {
+            if (def->os.firmwareFeatures[i] == VIR_TRISTATE_BOOL_ABSENT)
+                continue;
 
-                virBufferAsprintf(buf, "<feature enabled='%s' name='%s'/>\n",
-                                  virTristateBoolTypeToString(def->os.firmwareFeatures[i]),
-                                  virDomainOsDefFirmwareFeatureTypeToString(i));
-            }
+            virBufferAsprintf(buf, "<feature enabled='%s' name='%s'/>\n",
+                              virTristateBoolTypeToString(def->os.firmwareFeatures[i]),
+                              virDomainOsDefFirmwareFeatureTypeToString(i));
+        }
 
-            virBufferAdjustIndent(buf, -2);
+        virBufferAdjustIndent(buf, -2);
 
-            virBufferAddLit(buf, "</firmware>\n");
-        } else {
-            virBufferAddLit(buf, "/>\n");
-        }
+        virBufferAddLit(buf, "</firmware>\n");
     }
 
     virBufferEscapeString(buf, "<init>%s</init>\n",
diff --git a/tests/qemuxml2argvdata/os-firmware-efi-no-enrolled-keys.xml b/tests/qemuxml2argvdata/os-firmware-efi-no-enrolled-keys.xml
index 8944ce70bb..352908f745 100644
--- a/tests/qemuxml2argvdata/os-firmware-efi-no-enrolled-keys.xml
+++ b/tests/qemuxml2argvdata/os-firmware-efi-no-enrolled-keys.xml
@@ -6,7 +6,7 @@
   <vcpu placement='static'>1</vcpu>
   <os firmware='efi'>
     <type arch='x86_64' machine='pc-q35-4.0'>hvm</type>
-    <firmware type='efi'>
+    <firmware>
       <feature enabled='no' name='enrolled-keys'/>
     </firmware>
     <boot dev='hd'/>
diff --git a/tests/qemuxml2argvdata/os-firmware-invalid-type.xml b/tests/qemuxml2argvdata/os-firmware-invalid-type.xml
deleted file mode 100644
index 41360df0f7..0000000000
--- a/tests/qemuxml2argvdata/os-firmware-invalid-type.xml
+++ /dev/null
@@ -1,28 +0,0 @@
-<domain type='kvm'>
-  <name>fedora</name>
-  <uuid>63840878-0deb-4095-97e6-fc444d9bc9fa</uuid>
-  <memory unit='KiB'>8192</memory>
-  <currentMemory unit='KiB'>8192</currentMemory>
-  <vcpu placement='static'>1</vcpu>
-  <os firmware='efi'>
-    <type arch='x86_64' machine='pc-q35-4.0'>hvm</type>
-    <firmware type='bios'/>
-    <loader secure='no'/>
-    <nvram>/var/lib/libvirt/qemu/nvram/fedora_VARS.fd</nvram>
-    <boot dev='hd'/>
-    <bootmenu enable='yes'/>
-  </os>
-  <features>
-    <acpi/>
-    <apic/>
-    <pae/>
-  </features>
-  <clock offset='utc'/>
-  <on_poweroff>destroy</on_poweroff>
-  <on_reboot>restart</on_reboot>
-  <on_crash>restart</on_crash>
-  <devices>
-    <emulator>/usr/bin/qemu-system-x86_64</emulator>
-    <memballoon model='none'/>
-  </devices>
-</domain>
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 12c2b68fd7..3439f34ef1 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -3582,7 +3582,6 @@ mymain(void)
     DO_TEST_CAPS_LATEST("os-firmware-efi");
     DO_TEST_CAPS_LATEST("os-firmware-efi-secboot");
     DO_TEST_CAPS_LATEST("os-firmware-efi-no-enrolled-keys");
-    DO_TEST_CAPS_LATEST_PARSE_ERROR("os-firmware-invalid-type");
     DO_TEST_CAPS_ARCH_LATEST("aarch64-os-firmware-efi", "aarch64");
 
     DO_TEST_CAPS_LATEST("vhost-user-vga");
diff --git a/tests/qemuxml2xmloutdata/aarch64-os-firmware-efi.aarch64-latest.xml b/tests/qemuxml2xmloutdata/aarch64-os-firmware-efi.aarch64-latest.xml
index cb4f3ccfce..627e285ae1 100644
--- a/tests/qemuxml2xmloutdata/aarch64-os-firmware-efi.aarch64-latest.xml
+++ b/tests/qemuxml2xmloutdata/aarch64-os-firmware-efi.aarch64-latest.xml
@@ -6,7 +6,6 @@
   <vcpu placement='static'>1</vcpu>
   <os firmware='efi'>
     <type arch='aarch64' machine='virt-4.0'>hvm</type>
-    <firmware type='efi'/>
     <kernel>/aarch64.kernel</kernel>
     <initrd>/aarch64.initrd</initrd>
     <cmdline>earlyprintk console=ttyAMA0,115200n8 rw root=/dev/vda rootwait</cmdline>
diff --git a/tests/qemuxml2xmloutdata/os-firmware-bios.x86_64-latest.xml b/tests/qemuxml2xmloutdata/os-firmware-bios.x86_64-latest.xml
index 016c5b863f..df6f61421a 100644
--- a/tests/qemuxml2xmloutdata/os-firmware-bios.x86_64-latest.xml
+++ b/tests/qemuxml2xmloutdata/os-firmware-bios.x86_64-latest.xml
@@ -6,7 +6,6 @@
   <vcpu placement='static'>1</vcpu>
   <os firmware='bios'>
     <type arch='x86_64' machine='pc-q35-4.0'>hvm</type>
-    <firmware type='bios'/>
     <loader secure='no'/>
     <nvram>/var/lib/libvirt/qemu/nvram/fedora_VARS.fd</nvram>
     <boot dev='hd'/>
diff --git a/tests/qemuxml2xmloutdata/os-firmware-efi-secboot.x86_64-latest.xml b/tests/qemuxml2xmloutdata/os-firmware-efi-secboot.x86_64-latest.xml
index fa5eaa3148..c383546cc6 100644
--- a/tests/qemuxml2xmloutdata/os-firmware-efi-secboot.x86_64-latest.xml
+++ b/tests/qemuxml2xmloutdata/os-firmware-efi-secboot.x86_64-latest.xml
@@ -6,7 +6,6 @@
   <vcpu placement='static'>1</vcpu>
   <os firmware='efi'>
     <type arch='x86_64' machine='pc-q35-4.0'>hvm</type>
-    <firmware type='efi'/>
     <loader secure='yes'/>
     <nvram>/var/lib/libvirt/qemu/nvram/fedora_VARS.fd</nvram>
     <boot dev='hd'/>
diff --git a/tests/qemuxml2xmloutdata/os-firmware-efi.x86_64-latest.xml b/tests/qemuxml2xmloutdata/os-firmware-efi.x86_64-latest.xml
index 382146c23b..04d57860e7 100644
--- a/tests/qemuxml2xmloutdata/os-firmware-efi.x86_64-latest.xml
+++ b/tests/qemuxml2xmloutdata/os-firmware-efi.x86_64-latest.xml
@@ -6,7 +6,6 @@
   <vcpu placement='static'>1</vcpu>
   <os firmware='efi'>
     <type arch='x86_64' machine='pc-q35-4.0'>hvm</type>
-    <firmware type='efi'/>
     <loader secure='no'/>
     <nvram>/var/lib/libvirt/qemu/nvram/fedora_VARS.fd</nvram>
     <boot dev='hd'/>
diff --git a/tests/vmx2xmldata/vmx2xml-firmware-efi.xml b/tests/vmx2xmldata/vmx2xml-firmware-efi.xml
index fa10daf3a6..fee707fe71 100644
--- a/tests/vmx2xmldata/vmx2xml-firmware-efi.xml
+++ b/tests/vmx2xmldata/vmx2xml-firmware-efi.xml
@@ -6,7 +6,6 @@
   <vcpu placement='static'>1</vcpu>
   <os firmware='efi'>
     <type arch='i686'>hvm</type>
-    <firmware type='efi'/>
   </os>
   <clock offset='utc'/>
   <on_poweroff>destroy</on_poweroff>
-- 
2.30.2
Re: [libvirt PATCH] conf: remove duplicated firmware type attribute
Posted by Pavel Hrdina 3 years ago 
On Mon, Mar 29, 2021 at 07:03:52PM +0100, Daniel P. Berrangé wrote:
> The
> 
>   <os firmware='efi'>
>     <firmware type='efi'>
>       <feature enabled='no' name='enrolled-keys'/>
>     </firmware>
>   </os>
> 
> repeats the firmware attribute twice. This has no functional benefit, as
> evidenced by fact that we use a single struct field to store both
> attributes, while needlessly introducing an error scenario. The XML can
> just be simplified to:
> 
>   <os firmware='efi'>
>     <firmware>
>       <feature enabled='no' name='enrolled-keys'/>
>     </firmware>
>   </os>
> 
> which also means that we don't need to emit the empty element
> <firmware type='efi'/> for all existing configs too.

My original motivation was that if we ever need to introduce another
attribute to the <firmware> element it would be nicely grouped together.
But I guess it wound not be a big deal if we would have:

    <os firmware='efi'>
      <firmware someAttr='value'>
        <feature enabled='no' name='enrolled-keys'/>
      </firmware>
    </os>

This would look reasonable as well so

Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Re: [libvirt PATCH] conf: remove duplicated firmware type attribute
Posted by Daniel P. Berrangé 3 years ago 
On Mon, Mar 29, 2021 at 08:20:32PM +0200, Pavel Hrdina wrote:
> On Mon, Mar 29, 2021 at 07:03:52PM +0100, Daniel P. Berrangé wrote:
> > The
> > 
> >   <os firmware='efi'>
> >     <firmware type='efi'>
> >       <feature enabled='no' name='enrolled-keys'/>
> >     </firmware>
> >   </os>
> > 
> > repeats the firmware attribute twice. This has no functional benefit, as
> > evidenced by fact that we use a single struct field to store both
> > attributes, while needlessly introducing an error scenario. The XML can
> > just be simplified to:
> > 
> >   <os firmware='efi'>
> >     <firmware>
> >       <feature enabled='no' name='enrolled-keys'/>
> >     </firmware>
> >   </os>
> > 
> > which also means that we don't need to emit the empty element
> > <firmware type='efi'/> for all existing configs too.
> 
> My original motivation was that if we ever need to introduce another
> attribute to the <firmware> element it would be nicely grouped together.
> But I guess it wound not be a big deal if we would have:
> 
>     <os firmware='efi'>
>       <firmware someAttr='value'>
>         <feature enabled='no' name='enrolled-keys'/>
>       </firmware>
>     </os>
> 
> This would look reasonable as well so

This is pretty much the situation we're already in for a number of existing
elements too. For example <disk> has type=file|block|network on the top
level, but then the variable content is under the child <source> element.

If we didnt already have @firmware on the <os> element it would make sense
to have @type on <firmware>, but given existing schema, avoiding duplicating
information is better.

> Reviewed-by: Pavel Hrdina <phrdina@redhat.com>



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 :|

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.