[PATCH] domain_conf: include checks against dom->os.type and dom->virtType

[Ján Tomko]

On a Saturday in 2022, Jamm02 wrote:
>All the checks against dom->os.type and dom->virtType in
>virDomainInputDefParseXML shifted to
>virDomainInputDefValidate function
>

The code you're adding is not validation, it is the logic for filling
the default bus. *Validate functions should not alter the device
configuration.

A virDomainDefPostParseInput function would be more appropriate for this
code.

>Signed-off-by: Jamm02 <codeguy.moteen at gmail.com>
>---
> src/conf/domain_conf.c | 49 ++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 49 insertions(+)
>
>diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>index e0dfc9e45f..b237fd9ab1 100644
>--- a/src/conf/domain_conf.c
>+++ b/src/conf/domain_conf.c
>@@ -11837,6 +11837,55 @@ virDomainPanicDefParseXML(virDomainXMLOption *xmlopt,
>     return NULL;
> }
>
>+static virDomainInputDef *
>+virDomainInputDefValidate(const virDomainDef *dom,
>+                          xmlNodePtr node,
>+                          xmlXPathContextPtr ctxt)

Only the *Parse functions should be dealing with XML directly.

The later phases (PostParse and Validate) should operate on
virDomainInputDef.

Also, this function you're introducing is not used anywhere
and returns an incompletely parsed virDomainInputDef, which is not
helpful. Instead, it should return error/success (-1 / 0)

>+{
>+    VIR_XPATH_NODE_AUTORESTORE(ctxt)
>+    virDomainInputDef *def;
>+    g_autofree char *bus = NULL;
>+
>+    def = g_new0(virDomainInputDef, 1);
>+
>+    ctxt->node = node;
>+    bus = virXMLPropString(node, "bus");
>+
>+    if (bus) {
>+     if ((def->bus = virDomainInputBusTypeFromString(bus)) < 0) {
>+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>+                           _("unknown input bus type '%s'"), bus);
>+            goto error;
>+        }

This should stay in the parser. To be able to tell later whether there
was a bus specified in the XML, a new enum value
VIR_DOMAIN_INPUT_BUS_DEFAULT can be added.

>+
>+    } 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;
>+        }
>+    }

On success, the control flow would continue to the error label below and
always return NULL.

Jano

>+
>+ error:
>+    virDomainInputDefFree(def);
>+    return NULL;
>+}
> /* Parse the XML definition for an input device */
> static virDomainInputDef *
> virDomainInputDefParseXML(virDomainXMLOption *xmlopt,
>-- 
>2.35.1
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20220321/5d66e4f7/attachment.sig>