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

Jamm02 posted 1 patch 2 years, 1 month ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20220319132329.87041-1-codeguy.moteen@gmail.com
src/conf/domain_conf.c | 49 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 49 insertions(+)
[PATCH] domain_conf: include checks against dom->os.type and dom->virtType
Posted by Jamm02 2 years, 1 month ago
All the checks against dom->os.type and dom->virtType in
virDomainInputDefParseXML shifted to
virDomainInputDefValidate function

Signed-off-by: Jamm02 <codeguy.moteen@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)
+{
+    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;
+        }
+
+    } 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;
+        }
+    }
+
+ error:
+    virDomainInputDefFree(def);
+    return NULL;
+}
 /* Parse the XML definition for an input device */
 static virDomainInputDef *
 virDomainInputDefParseXML(virDomainXMLOption *xmlopt,
-- 
2.35.1
Re: [PATCH] domain_conf: include checks against dom->os.type and dom->virtType
Posted by Ján Tomko 2 years, 1 month ago
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@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
>