It only has a single caller.

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
 src/qemu/qemu_domain.c         |  4 +++-
 src/qemu/qemu_domain_address.c | 25 -------------------------
 src/qemu/qemu_domain_address.h |  4 ----
 3 files changed, 3 insertions(+), 30 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 9289c1fa18..48d721a809 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -5584,7 +5584,9 @@ qemuDomainControllerDefPostParse(virDomainControllerDef *cont,
     switch (cont->type) {
     case VIR_DOMAIN_CONTROLLER_TYPE_SCSI:
         /* Set the default SCSI controller model if not already set */
-        if (qemuDomainSetSCSIControllerModel(def, cont, qemuCaps) < 0)
+        cont->model = qemuDomainGetSCSIControllerModel(def, cont, qemuCaps);
+
+        if (cont->model < 0)
             return -1;
         break;
 
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index 22af903b6a..0a7273dc07 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -74,31 +74,6 @@ qemuDomainGetSCSIControllerModel(const virDomainDef *def,
 }
 
 
-/**
- * @def: Domain definition
- * @cont: Domain controller def
- * @qemuCaps: qemu capabilities
- *
- * Set the controller model based on the existing value and the
- * capabilities if possible.
- *
- * Returns 0 on success, -1 on failure with error set.
- */
-int
-qemuDomainSetSCSIControllerModel(const virDomainDef *def,
-                                 virDomainControllerDef *cont,
-                                 virQEMUCaps *qemuCaps)
-{
-    int model = qemuDomainGetSCSIControllerModel(def, cont, qemuCaps);
-
-    if (model < 0)
-        return -1;
-
-    cont->model = model;
-    return 0;
-}
-
-
 static int
 qemuDomainAssignVirtioSerialAddresses(virDomainDef *def)
 {
diff --git a/src/qemu/qemu_domain_address.h b/src/qemu/qemu_domain_address.h
index a571312469..0ac6285f2e 100644
--- a/src/qemu/qemu_domain_address.h
+++ b/src/qemu/qemu_domain_address.h
@@ -28,10 +28,6 @@ int qemuDomainGetSCSIControllerModel(const virDomainDef *def,
                                      const virDomainControllerDef *cont,
                                      virQEMUCaps *qemuCaps);
 
-int qemuDomainSetSCSIControllerModel(const virDomainDef *def,
-                                     virDomainControllerDef *cont,
-                                     virQEMUCaps *qemuCaps);
-
 int qemuDomainAssignAddresses(virDomainDef *def,
                               virQEMUCaps *qemuCaps,
                               virQEMUDriver *driver,
-- 
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:33 +0100, Andrea Bolognani wrote:
> It only has a single caller.
> 
> Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> ---
>  src/qemu/qemu_domain.c         |  4 +++-
>  src/qemu/qemu_domain_address.c | 25 -------------------------
>  src/qemu/qemu_domain_address.h |  4 ----
>  3 files changed, 3 insertions(+), 30 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 9289c1fa18..48d721a809 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -5584,7 +5584,9 @@ qemuDomainControllerDefPostParse(virDomainControllerDef *cont,
>      switch (cont->type) {
>      case VIR_DOMAIN_CONTROLLER_TYPE_SCSI:
>          /* Set the default SCSI controller model if not already set */
> -        if (qemuDomainSetSCSIControllerModel(def, cont, qemuCaps) < 0)
> +        cont->model = qemuDomainGetSCSIControllerModel(def, cont, qemuCaps);
> +
> +        if (cont->model < 0)
>              return -1;

This is not future proof for the case when 'model' gets turned into a
proper enum.
_______________________________________________
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:15:14 +0100, Peter Krempa wrote:
> On Wed, Jan 24, 2024 at 20:37:33 +0100, Andrea Bolognani wrote:
> > It only has a single caller.
> > 
> > Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> > ---
> >  src/qemu/qemu_domain.c         |  4 +++-
> >  src/qemu/qemu_domain_address.c | 25 -------------------------
> >  src/qemu/qemu_domain_address.h |  4 ----
> >  3 files changed, 3 insertions(+), 30 deletions(-)
> > 
> > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > index 9289c1fa18..48d721a809 100644
> > --- a/src/qemu/qemu_domain.c
> > +++ b/src/qemu/qemu_domain.c
> > @@ -5584,7 +5584,9 @@ qemuDomainControllerDefPostParse(virDomainControllerDef *cont,
> >      switch (cont->type) {
> >      case VIR_DOMAIN_CONTROLLER_TYPE_SCSI:
> >          /* Set the default SCSI controller model if not already set */
> > -        if (qemuDomainSetSCSIControllerModel(def, cont, qemuCaps) < 0)
> > +        cont->model = qemuDomainGetSCSIControllerModel(def, cont, qemuCaps);
> > +
> > +        if (cont->model < 0)
> >              return -1;
> 
> This is not future proof for the case when 'model' gets turned into a
> proper enum.

Based on further patches, it seems to be a problem in more places.

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 11:17:46AM +0100, Peter Krempa wrote:
> On Thu, Jan 25, 2024 at 11:15:14 +0100, Peter Krempa wrote:
> > On Wed, Jan 24, 2024 at 20:37:33 +0100, Andrea Bolognani wrote:
> > > @@ -5584,7 +5584,9 @@ qemuDomainControllerDefPostParse(virDomainControllerDef *cont,
> > >      switch (cont->type) {
> > >      case VIR_DOMAIN_CONTROLLER_TYPE_SCSI:
> > >          /* Set the default SCSI controller model if not already set */
> > > -        if (qemuDomainSetSCSIControllerModel(def, cont, qemuCaps) < 0)
> > > +        cont->model = qemuDomainGetSCSIControllerModel(def, cont, qemuCaps);
> > > +
> > > +        if (cont->model < 0)
> > >              return -1;
> >
> > This is not future proof for the case when 'model' gets turned into a
> > proper enum.
>
> Based on further patches, it seems to be a problem in more places.

I'm not sure I understand. Can you please explain what "a proper
enum" means in this context? Is that connected to some condition that
could present itself in the current code, or as a consequence of a
potential future refactoring? Based on the fact that you ultimately
ACKed the patch, I assume the latter.

-- 
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:16:55 -0800, Andrea Bolognani wrote:
> On Thu, Jan 25, 2024 at 11:17:46AM +0100, Peter Krempa wrote:
> > On Thu, Jan 25, 2024 at 11:15:14 +0100, Peter Krempa wrote:
> > > On Wed, Jan 24, 2024 at 20:37:33 +0100, Andrea Bolognani wrote:
> > > > @@ -5584,7 +5584,9 @@ qemuDomainControllerDefPostParse(virDomainControllerDef *cont,
> > > >      switch (cont->type) {
> > > >      case VIR_DOMAIN_CONTROLLER_TYPE_SCSI:
> > > >          /* Set the default SCSI controller model if not already set */
> > > > -        if (qemuDomainSetSCSIControllerModel(def, cont, qemuCaps) < 0)
> > > > +        cont->model = qemuDomainGetSCSIControllerModel(def, cont, qemuCaps);
> > > > +
> > > > +        if (cont->model < 0)
> > > >              return -1;
> > >
> > > This is not future proof for the case when 'model' gets turned into a
> > > proper enum.
> >
> > Based on further patches, it seems to be a problem in more places.
> 
> I'm not sure I understand. Can you please explain what "a proper
> enum" means in this context? Is that connected to some condition that

If you turn the 'model' member from 'int' to the appropriate enum type
the '< 0' check will no longer work if the enum has no negative members.
This is due to the fact that the compiler then treats the enum as
'unsigned'

> could present itself in the current code, or as a consequence of a
> potential future refactoring? Based on the fact that you ultimately
> ACKed the patch, I assume the latter.

There are multiple other instances of exactly the same problem for this
enum and additionally this one has a native -1 member, so my point was
moot mostly.
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org