Factor out the existing code.

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
 src/qemu/qemu_domain.c | 66 ++++++++++++++++++++++--------------------
 1 file changed, 35 insertions(+), 31 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index b3e63b3648..c9d213dd7b 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4312,6 +4312,40 @@ qemuDomainDefaultSerialType(const virDomainDef *def)
 }
 
 
+static int
+qemuDomainDefaultSerialModel(const virDomainDef *def,
+                             const virDomainChrDef *chr)
+{
+    switch ((virDomainChrSerialTargetType)chr->targetType) {
+    case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA:
+        return VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_SERIAL;
+    case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_USB:
+        return VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_USB_SERIAL;
+    case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_PCI:
+        return VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_PCI_SERIAL;
+    case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SPAPR_VIO:
+        return VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_SPAPR_VTY;
+    case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SYSTEM:
+        if (qemuDomainIsARMVirt(def)) {
+            return VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_PL011;
+        } else if (qemuDomainIsRISCVVirt(def)) {
+            return VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_16550A;
+        }
+        break;
+    case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SCLP:
+        return VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_SCLPCONSOLE;
+    case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA_DEBUG:
+        return VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_DEBUGCON;
+    case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_NONE:
+    case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_LAST:
+        /* Nothing to do */
+        break;
+    }
+
+    return VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_NONE;
+}
+
+
 static int
 qemuDomainDefAddDefaultDevices(virQEMUDriver *driver,
                                virDomainDef *def,
@@ -5898,37 +5932,7 @@ qemuDomainChrDefPostParse(virDomainChrDef *chr,
     /* Set the default target model */
     if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL &&
         chr->targetModel == VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_NONE) {
-        switch ((virDomainChrSerialTargetType)chr->targetType) {
-        case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA:
-            chr->targetModel = VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_SERIAL;
-            break;
-        case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_USB:
-            chr->targetModel = VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_USB_SERIAL;
-            break;
-        case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_PCI:
-            chr->targetModel = VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_PCI_SERIAL;
-            break;
-        case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SPAPR_VIO:
-            chr->targetModel = VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_SPAPR_VTY;
-            break;
-        case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SYSTEM:
-            if (qemuDomainIsARMVirt(def)) {
-                chr->targetModel = VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_PL011;
-            } else if (qemuDomainIsRISCVVirt(def)) {
-                chr->targetModel = VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_16550A;
-            }
-            break;
-        case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SCLP:
-            chr->targetModel = VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_SCLPCONSOLE;
-            break;
-        case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA_DEBUG:
-            chr->targetModel = VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_DEBUGCON;
-            break;
-        case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_NONE:
-        case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_LAST:
-            /* Nothing to do */
-            break;
-        }
+        chr->targetModel = qemuDomainDefaultSerialModel(def, chr);
     }
 
     /* clear auto generated unix socket path for inactive definitions */
-- 
2.43.0
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

On Wed, Jan 24, 2024 at 20:37:45 +0100, Andrea Bolognani wrote:
> Factor out the existing code.
> 
> Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> ---
>  src/qemu/qemu_domain.c | 66 ++++++++++++++++++++++--------------------
>  1 file changed, 35 insertions(+), 31 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index b3e63b3648..c9d213dd7b 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -4312,6 +4312,40 @@ qemuDomainDefaultSerialType(const virDomainDef *def)
>  }
>  
>  
> +static int

IMO at this point you can return virDomainChrSerialTargetModel.
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

On Thu, Jan 25, 2024 at 03:50:33PM +0100, Peter Krempa wrote:
> On Wed, Jan 24, 2024 at 20:37:45 +0100, Andrea Bolognani wrote:
> > +static int
>
> IMO at this point you can return virDomainChrSerialTargetModel.

Sure, I can try that. But what's the advantage? Ultimately we store
most (all?) models as int inside structs, and only cast them to the
proper enum type for switches. Which is a real shame, obviously, but
IIRC was done for Some Actual Reason™. Or am I misremembering?

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

On Thu, Jan 25, 2024 at 11:07:41 -0800, Andrea Bolognani wrote:
> On Thu, Jan 25, 2024 at 03:50:33PM +0100, Peter Krempa wrote:
> > On Wed, Jan 24, 2024 at 20:37:45 +0100, Andrea Bolognani wrote:
> > > +static int
> >
> > IMO at this point you can return virDomainChrSerialTargetModel.
> 
> Sure, I can try that. But what's the advantage? Ultimately we store
> most (all?) models as int inside structs, and only cast them to the

This pattern is discouraged and being actively elliminated. The main
reason for this pattern to exist was that the enum*TypeFromString
function returns -1 on error, so you can't directly assign it into a
enum type which is unsigned.

> proper enum type for switches. Which is a real shame, obviously, but
> IIRC was done for Some Actual Reason™. Or am I misremembering?

The additional reason here is that if somebody will be editting this
function and decide to add a return of an arbitrary  -1 or -2, returning
a proper enum type will make them reconsider what they are doing and
look at all callers before doing so.
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

On Wed, Jan 24, 2024 at 20:37:45 +0100, Andrea Bolognani wrote:
> Factor out the existing code.
> 
> Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> ---
>  src/qemu/qemu_domain.c | 66 ++++++++++++++++++++++--------------------
>  1 file changed, 35 insertions(+), 31 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index b3e63b3648..c9d213dd7b 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -4312,6 +4312,40 @@ qemuDomainDefaultSerialType(const virDomainDef *def)
>  }
>  
>  
> +static int
> +qemuDomainDefaultSerialModel(const virDomainDef *def,
> +                             const virDomainChrDef *chr)
> +{
> +    switch ((virDomainChrSerialTargetType)chr->targetType) {
> +    case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA:
> +        return VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_SERIAL;
> +    case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_USB:
> +        return VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_USB_SERIAL;
> +    case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_PCI:
> +        return VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_PCI_SERIAL;
> +    case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SPAPR_VIO:
> +        return VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_SPAPR_VTY;
> +    case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SYSTEM:
> +        if (qemuDomainIsARMVirt(def)) {
> +            return VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_PL011;
> +        } else if (qemuDomainIsRISCVVirt(def)) {
> +            return VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_16550A;
> +        }
> +        break;
> +    case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SCLP:
> +        return VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_SCLPCONSOLE;
> +    case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA_DEBUG:
> +        return VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_DEBUGCON;
> +    case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_NONE:
> +    case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_LAST:
> +        /* Nothing to do */
> +        break;
> +    }

Preferrably add visual separation around each case.


> +
> +    return VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_NONE;
> +}
> +
> +
>  static int

Reviewed-by: Peter Krempa <pkrempa@redhat.com>
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org