[PATCH 23/33] qemu: Enhance qemuDomainForbidLegacyUSBController()

Andrea Bolognani posted 33 patches 8 months, 2 weeks ago
There is a newer version of this series
[PATCH 23/33] qemu: Enhance qemuDomainForbidLegacyUSBController()
Posted by Andrea Bolognani 8 months, 2 weeks ago
Currently, we have special handling for USB controllers of
s390x guests hardcoded into the command line generator. This is
not great from a layering point of view and, given the complex
interactions between the various parts, just makes things very
confusing.

In order to make things easier to reason about and centralize
decision making, increase the number of possible return values.

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
 src/qemu/qemu_command.c |  5 ++---
 src/qemu/qemu_domain.c  | 28 +++++++++++++++++++++++++---
 src/qemu/qemu_domain.h  |  2 +-
 3 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index ec4982bbf6..deb8867938 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -2949,8 +2949,7 @@ qemuBuildLegacyUSBControllerCommandLine(virCommand *cmd,
     }
 
     if (nusb == 0 &&
-        !qemuDomainForbidLegacyUSBController(def) &&
-        !ARCH_IS_S390(def->os.arch)) {
+        qemuDomainForbidLegacyUSBController(def) == 0) {
         /* We haven't added any USB controller yet, but we haven't been asked
          * not to add one either. Add a legacy USB controller, unless we're
          * creating a kind of guest we want to keep legacy-free */
@@ -3049,7 +3048,7 @@ qemuBuildControllersByTypeCommandLine(virCommand *cmd,
 
         if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_USB &&
             cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT &&
-            !qemuDomainForbidLegacyUSBController(def)) {
+            qemuDomainForbidLegacyUSBController(def) <= 0) {
 
             /* An appropriate default USB controller model should already
              * have been selected in qemuDomainDeviceDefPostParse(); if
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 2b7eae295b..5b93529655 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4258,15 +4258,37 @@ qemuDomainDefaultUSBControllerModel(const virDomainDef *def,
 }
 
 
-bool
+/**
+ * qemuDomainForbidLegacyUSBController:
+ * @def: domain definition
+ *
+ * Tells the command line generator whether it's acceptable to fall
+ * back to the legacy '-usb' option when a specific model hasn't been
+ * provided or automatically selected for the USB controller.
+ *
+ * Returns:  0 if '-usb' can be used,
+ *          >0 if '-usb' should be avoided, and
+ *          <0 if '-usb' should't be used but the fact that we have
+ *             reached the point where that was the only remaining
+ *             option shouldn't be considered an overall failure.
+ */
+int
 qemuDomainForbidLegacyUSBController(const virDomainDef *def)
 {
+    /* Modern guests should never use the legacy controller */
     if (qemuDomainIsQ35(def) ||
         qemuDomainIsARMVirt(def) ||
         qemuDomainIsRISCVVirt(def))
-        return true;
+        return 1;
 
-    return false;
+    /* In the case of s390x, we also never want to use the legacy
+     * controller but it's okay if that means that we end up creating
+     * no USB controller at all */
+    if (ARCH_IS_S390(def->os.arch))
+        return -1;
+
+    /* In all other cases, using the legacy controller is okay */
+    return 0;
 }
 
 
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 4e61d741f3..ff95c392bd 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -839,7 +839,7 @@ bool qemuDomainSupportsPCIMultibus(const virDomainDef *def);
 int qemuDomainDefaultSCSIControllerModel(const virDomainDef *def,
                                          const virDomainControllerDef *cont,
                                          virQEMUCaps *qemuCaps);
-bool qemuDomainForbidLegacyUSBController(const virDomainDef *def);
+int qemuDomainForbidLegacyUSBController(const virDomainDef *def);
 
 void qemuDomainUpdateCurrentMemorySize(virDomainObj *vm);
 
-- 
2.43.0
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
Re: [PATCH 23/33] qemu: Enhance qemuDomainForbidLegacyUSBController()
Posted by Peter Krempa 8 months, 2 weeks ago
On Wed, Jan 24, 2024 at 20:37:43 +0100, Andrea Bolognani wrote:
> Currently, we have special handling for USB controllers of
> s390x guests hardcoded into the command line generator. This is
> not great from a layering point of view and, given the complex
> interactions between the various parts, just makes things very
> confusing.
> 
> In order to make things easier to reason about and centralize
> decision making, increase the number of possible return values.

Honestly, to centralize decision making, the commandline code should not
at all call this function but simply rely on pre-filled list of
controllers. If the list is empty or contains a _NONE controller, don't
format anything. for _DEFAULT it should do '-usb'. Errors and anything
else should be decided before.

IMO this patch doesn't do anything for that.

For this series you should be able to separate the USB-unrelated changes
and get them merged.

If you want to have a look at doing this properly then go ahead,
otherwise I'll try having a look.
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
Re: Re: [PATCH 23/33] qemu: Enhance qemuDomainForbidLegacyUSBController()
Posted by Andrea Bolognani 8 months, 2 weeks ago
On Thu, Jan 25, 2024 at 04:45:19PM +0100, Peter Krempa wrote:
> On Wed, Jan 24, 2024 at 20:37:43 +0100, Andrea Bolognani wrote:
> > Currently, we have special handling for USB controllers of
> > s390x guests hardcoded into the command line generator. This is
> > not great from a layering point of view and, given the complex
> > interactions between the various parts, just makes things very
> > confusing.
> >
> > In order to make things easier to reason about and centralize
> > decision making, increase the number of possible return values.
>
> Honestly, to centralize decision making, the commandline code should not
> at all call this function but simply rely on pre-filled list of
> controllers. If the list is empty or contains a _NONE controller, don't
> format anything. for _DEFAULT it should do '-usb'. Errors and anything
> else should be decided before.
>
> IMO this patch doesn't do anything for that.
>
> For this series you should be able to separate the USB-unrelated changes
> and get them merged.
>
> If you want to have a look at doing this properly then go ahead,
> otherwise I'll try having a look.

I can try looking into it, but I'm not convinced that's going to
improve things as much as you hope. I'd love to be proven wrong
though :)

-- 
Andrea Bolognani / Red Hat / Virtualization
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org