The function is modified to be stateless, which is more
consistent with existing helpers that deal with figuring out
default models for devices, and its name needs to change
accordingly.

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
 src/qemu/qemu_domain.c  | 21 +++++++++++----------
 src/qemu/qemu_domain.h  |  6 +++---
 src/qemu/qemu_hotplug.c |  5 +++--
 3 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 672f1ce59f..97336d5002 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4106,23 +4106,23 @@ qemuDomainDefAddDefaultAudioBackend(virQEMUDriver *driver,
 
 
 /**
+ * qemuDomainDefaultSCSIControllerModel:
  * @def: Domain definition
  * @cont: Domain controller def
  * @qemuCaps: qemu capabilities
  *
- * If the controller model is already defined, return it immediately;
- * otherwise, based on the @qemuCaps return a default model value.
+ * Decides the preferred model for a SCSI controller that it to be
+ * added to @def. The choice might be based on various factors,
+ * including the architecture, machine type and what devices are
+ * available according to @qemuCaps.
  *
  * Returns model on success, -1 on failure with error set.
  */
 int
-qemuDomainGetSCSIControllerModel(const virDomainDef *def,
-                                 const virDomainControllerDef *cont,
-                                 virQEMUCaps *qemuCaps)
+qemuDomainDefaultSCSIControllerModel(const virDomainDef *def,
+                                     const virDomainControllerDef *cont,
+                                     virQEMUCaps *qemuCaps)
 {
-    if (cont->model > 0)
-        return cont->model;
-
     if (qemuDomainIsPSeries(def))
         return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI;
     if (ARCH_IS_S390(def->os.arch))
@@ -5621,9 +5621,10 @@ qemuDomainControllerDefPostParse(virDomainControllerDef *cont,
     switch (cont->type) {
     case VIR_DOMAIN_CONTROLLER_TYPE_SCSI:
         /* Set the default SCSI controller model if not already set */
-        cont->model = qemuDomainGetSCSIControllerModel(def, cont, qemuCaps);
+        if (cont->model <= 0)
+            cont->model = qemuDomainDefaultSCSIControllerModel(def, cont, qemuCaps);
 
-        if (cont->model < 0)
+        if (cont->model <= 0)
             return -1;
         break;
 
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 4dfc0ab5c7..48f966fa2a 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -836,9 +836,9 @@ bool qemuDomainHasBuiltinESP(const virDomainDef *def);
 bool qemuDomainNeedsFDC(const virDomainDef *def);
 bool qemuDomainSupportsPCI(const virDomainDef *def);
 bool qemuDomainSupportsPCIMultibus(const virDomainDef *def);
-int qemuDomainGetSCSIControllerModel(const virDomainDef *def,
-                                     const virDomainControllerDef *cont,
-                                     virQEMUCaps *qemuCaps);
+int qemuDomainDefaultSCSIControllerModel(const virDomainDef *def,
+                                         const virDomainControllerDef *cont,
+                                         virQEMUCaps *qemuCaps);
 
 void qemuDomainUpdateCurrentMemorySize(virDomainObj *vm);
 
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index c883fef0e9..4fd7b7f756 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -878,9 +878,10 @@ qemuDomainFindOrCreateSCSIDiskController(virDomainObj *vm,
     cont->idx = controller;
     cont->model = model;
 
-    cont->model = qemuDomainGetSCSIControllerModel(vm->def, cont, priv->qemuCaps);
+    if (cont->model <= 0)
+        cont->model = qemuDomainDefaultSCSIControllerModel(vm->def, cont, priv->qemuCaps);
 
-    if (cont->model < 0) {
+    if (cont->model <= 0) {
         VIR_FREE(cont);
         return NULL;
     }
-- 
2.43.0
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

Summary states just "Rename"

On Wed, Jan 24, 2024 at 20:37:37 +0100, Andrea Bolognani wrote:
> The function is modified to be stateless, which is more
> consistent with existing helpers that deal with figuring out
> default models for devices, and its name needs to change
> accordingly.

But logic is changed. Don't mislead in the summary. It's acceptable to
rename the function as side effect of logic changes rather than have
logic changes as side effect of 'rename'

> Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> ---
>  src/qemu/qemu_domain.c  | 21 +++++++++++----------
>  src/qemu/qemu_domain.h  |  6 +++---
>  src/qemu/qemu_hotplug.c |  5 +++--
>  3 files changed, 17 insertions(+), 15 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 672f1ce59f..97336d5002 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -4106,23 +4106,23 @@ qemuDomainDefAddDefaultAudioBackend(virQEMUDriver *driver,
>  
>  
>  /**
> + * qemuDomainDefaultSCSIControllerModel:
>   * @def: Domain definition
>   * @cont: Domain controller def
>   * @qemuCaps: qemu capabilities
>   *
> - * If the controller model is already defined, return it immediately;
> - * otherwise, based on the @qemuCaps return a default model value.
> + * Decides the preferred model for a SCSI controller that it to be
> + * added to @def. The choice might be based on various factors,
> + * including the architecture, machine type and what devices are
> + * available according to @qemuCaps.
>   *
>   * Returns model on success, -1 on failure with error set.
>   */
>  int
> -qemuDomainGetSCSIControllerModel(const virDomainDef *def,
> -                                 const virDomainControllerDef *cont,
> -                                 virQEMUCaps *qemuCaps)
> +qemuDomainDefaultSCSIControllerModel(const virDomainDef *def,
> +                                     const virDomainControllerDef *cont,
> +                                     virQEMUCaps *qemuCaps)
>  {
> -    if (cont->model > 0)
> -        return cont->model;
> -
>      if (qemuDomainIsPSeries(def))
>          return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI;
>      if (ARCH_IS_S390(def->os.arch))
> @@ -5621,9 +5621,10 @@ qemuDomainControllerDefPostParse(virDomainControllerDef *cont,
>      switch (cont->type) {
>      case VIR_DOMAIN_CONTROLLER_TYPE_SCSI:
>          /* Set the default SCSI controller model if not already set */
> -        cont->model = qemuDomainGetSCSIControllerModel(def, cont, qemuCaps);
> +        if (cont->model <= 0)
> +            cont->model = qemuDomainDefaultSCSIControllerModel(def, cont, qemuCaps);
>  
> -        if (cont->model < 0)
> +        if (cont->model <= 0)

The function comment states:

>   *
>   * Returns model on success, -1 on failure with error set.
>   */

While '0' currently can't happen the check contradicts the documented
return value. The function documents that error is set only on -1 thus
returning '-1' here if qemuDomainDefaultSCSIControllerModel return 0
would propagate a failure without error reported.

Either you modify the function docs to state that also 0 is considered
error and error is to be set, or don't modify the checks.

Also this still is sub-optimal as
VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT is a valid enum member with
value of -1;

Obviously the preferrable solution would be to return the model via
pointer/argument and just return success error from the main function to
avoid the problems described above.

With the commit message fixed and one of:
 - remove the 'cont->model <= 0' changes
 - the comment describing the return values improved

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

On Thu, Jan 25, 2024 at 01:45:30PM +0100, Peter Krempa wrote:
> Summary states just "Rename"
> But logic is changed. Don't mislead in the summary. It's acceptable to
> rename the function as side effect of logic changes rather than have
> logic changes as side effect of 'rename'

You're right.

> >      case VIR_DOMAIN_CONTROLLER_TYPE_SCSI:
> >          /* Set the default SCSI controller model if not already set */
> > -        cont->model = qemuDomainGetSCSIControllerModel(def, cont, qemuCaps);
> > +        if (cont->model <= 0)
> > +            cont->model = qemuDomainDefaultSCSIControllerModel(def, cont, qemuCaps);
> >
> > -        if (cont->model < 0)
> > +        if (cont->model <= 0)
>
> The function comment states:
>
> >   *
> >   * Returns model on success, -1 on failure with error set.
> >   */
>
> While '0' currently can't happen the check contradicts the documented
> return value. The function documents that error is set only on -1 thus
> returning '-1' here if qemuDomainDefaultSCSIControllerModel return 0
> would propagate a failure without error reported.
>
> Either you modify the function docs to state that also 0 is considered
> error and error is to be set, or don't modify the checks.

Yeah, good catch.

> Also this still is sub-optimal as
> VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT is a valid enum member with
> value of -1;

That's true.

However, both DEFAULT and AUTO are effectively non-choices, and since
the purpose of these helpers is to resolve DEFAULT[*] to an actual
model, I think considering them as error conditions is only fair.

The alternative course of action, which IIRC is what happens for USB
controllers, is to accept DEFAULT in postparse only to reject it
further down the stack. I'm not convinced that's preferrable, but I
can give that approach a try if you want me to.


[*] I haven't really considered AUTO, and I'm not sure why it exists.
    I'll be sure to look into it.
-- 
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 10:34:54 -0800, Andrea Bolognani wrote:
> On Thu, Jan 25, 2024 at 01:45:30PM +0100, Peter Krempa wrote:
> > Summary states just "Rename"
> > But logic is changed. Don't mislead in the summary. It's acceptable to
> > rename the function as side effect of logic changes rather than have
> > logic changes as side effect of 'rename'
> 
> You're right.
> 
> > >      case VIR_DOMAIN_CONTROLLER_TYPE_SCSI:
> > >          /* Set the default SCSI controller model if not already set */
> > > -        cont->model = qemuDomainGetSCSIControllerModel(def, cont, qemuCaps);
> > > +        if (cont->model <= 0)
> > > +            cont->model = qemuDomainDefaultSCSIControllerModel(def, cont, qemuCaps);
> > >
> > > -        if (cont->model < 0)
> > > +        if (cont->model <= 0)
> >
> > The function comment states:
> >
> > >   *
> > >   * Returns model on success, -1 on failure with error set.
> > >   */
> >
> > While '0' currently can't happen the check contradicts the documented
> > return value. The function documents that error is set only on -1 thus
> > returning '-1' here if qemuDomainDefaultSCSIControllerModel return 0
> > would propagate a failure without error reported.
> >
> > Either you modify the function docs to state that also 0 is considered
> > error and error is to be set, or don't modify the checks.
> 
> Yeah, good catch.
> 
> > Also this still is sub-optimal as
> > VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT is a valid enum member with
> > value of -1;
> 
> That's true.
> 
> However, both DEFAULT and AUTO are effectively non-choices, and since
> the purpose of these helpers is to resolve DEFAULT[*] to an actual
> model, I think considering them as error conditions is only fair.

It can be considered an error but must not be described as "-1 is an
error" as -1 is a valid enum value. Based on the fact that libvirt uses
'-1' widely as an error value here it can mislead about the actual
meaninig.

That's why I've also asked to use the enum type as return value in
subsequent patches. IT's for anyone trying to change the function to
think about what they are doing, and seeing '-1 is an error' simply
evokes the default pattern we use which is not the case here.

> The alternative course of action, which IIRC is what happens for USB
> controllers, is to accept DEFAULT in postparse only to reject it
> further down the stack. I'm not convinced that's preferrable, but I
> can give that approach a try if you want me to.
> 
> 
> [*] I haven't really considered AUTO, and I'm not sure why it exists.
>     I'll be sure to look into it.
> -- 
> Andrea Bolognani / Red Hat / Virtualization
> 
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org