Re: [libvirt PATCH] Move default Input bus logic to PostParse handling

[Martin Kletzander]

On Sat, Apr 22, 2023 at 11:25:30PM +0530, K Shiva wrote:

A new enum type "Default" has been added for Input bus.
The logic that handled default input bus types in
virDomainInputParseXML() has been moved to a new function
virDomainInputDefPostParse() in domain_postparse.c
Link to Issue: https://gitlab.com/libvirt/libvirt/-/issues/8

Signed-off-by: K Shiva <shiva...@riseup.net>
---
src/conf/domain_conf.c         | 27 +++------------------------
src/conf/domain_conf.h         |  1 +
src/conf/domain_postparse.c    | 30 ++++++++++++++++++++++++++++++
src/qemu/qemu_command.c        |  1 +
src/qemu/qemu_domain_address.c |  1 +
src/qemu/qemu_hotplug.c        |  2 ++
6 files changed, 38 insertions(+), 24 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index b03a3ff011..22af3f1d8a 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -905,6 +905,7 @@ VIR_ENUM_IMPL(virDomainInput,

VIR_ENUM_IMPL(virDomainInputBus,
              VIR_DOMAIN_INPUT_BUS_LAST,
+              "default",
              "ps2",
              "usb",
              "xen",
@@ -10693,7 +10694,6 @@ virDomainPanicDefParseXML(virDomainXMLOption *xmlopt,
/* Parse the XML definition for an input device */
static virDomainInputDef *
virDomainInputDefParseXML(virDomainXMLOption *xmlopt,
-                          const virDomainDef *dom,
                          xmlNodePtr node,
                          xmlXPathContextPtr ctxt,
                          unsigned int flags)
@@ -10741,27 +10741,7 @@ virDomainInputDefParseXML(virDomainXMLOption *xmlopt,
        }

    } else {
-        if (dom->os.type == VIR_DOMAIN_OSTYPE_HVM) {
-            if ((def->type == VIR_DOMAIN_INPUT_TYPE_MOUSE ||
-                def->type == VIR_DOMAIN_INPUT_TYPE_KBD) &&
-                (ARCH_IS_X86(dom->os.arch) || dom->os.arch == VIR_ARCH_NONE)) {
-                def->bus = VIR_DOMAIN_INPUT_BUS_PS2;
-            } else if (ARCH_IS_S390(dom->os.arch) ||
-                       def->type == VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH) {
-                def->bus = VIR_DOMAIN_INPUT_BUS_VIRTIO;
-            } else if (def->type == VIR_DOMAIN_INPUT_TYPE_EVDEV) {
-                def->bus = VIR_DOMAIN_INPUT_BUS_NONE;
-            } else {
-                def->bus = VIR_DOMAIN_INPUT_BUS_USB;
-            }
-        } else if (dom->os.type == VIR_DOMAIN_OSTYPE_XEN ||
-                   dom->os.type == VIR_DOMAIN_OSTYPE_XENPVH) {
-            def->bus = VIR_DOMAIN_INPUT_BUS_XEN;
-        } else {
-            if ((dom->virtType == VIR_DOMAIN_VIRT_VZ ||
-                 dom->virtType == VIR_DOMAIN_VIRT_PARALLELS))
-                def->bus = VIR_DOMAIN_INPUT_BUS_PARALLELS;
-        }
+        def->bus = VIR_DOMAIN_INPUT_BUS_DEFAULT;

Since @def here is zero-initialized and _BUS_DEFAULT is 0 (the first
variant of that enum) you can skip this branch completely.  And if you
look at the model parsing above, we should ideally forbid the user to
set bus="default" too.

Reviewed-by: Martin Kletzander <mklet...@redhat.com>

And I would squash this in before pushing if you're OK with that:

diff --git i/src/conf/domain_conf.c w/src/conf/domain_conf.c
index 0d8b128f9538..222dd989f54f 100644
--- i/src/conf/domain_conf.c
+++ w/src/conf/domain_conf.c
@@ -10733,15 +10733,12 @@ virDomainInputDefParseXML(virDomainXMLOption *xmlopt,
         goto error;
     }

-    if (bus) {
-        if ((def->bus = virDomainInputBusTypeFromString(bus)) < 0) {
-            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                           _("unknown input bus type '%1$s'"), bus);
-            goto error;
-        }
-
-    } else {
-        def->bus = VIR_DOMAIN_INPUT_BUS_DEFAULT;
+    if (bus &&
+        ((def->bus = virDomainInputBusTypeFromString(bus)) < 0 ||
+         def->bus == VIR_DOMAIN_INPUT_BUS_DEFAULT)) {
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                       _("unknown input bus type '%1$s'"), bus);
+        goto error;
     }

     if (virDomainDeviceInfoParseXML(xmlopt, node, ctxt, &def->info, flags) < 0)
--

Martin

signature.asc
Description: PGP signature