Not a massive difference, but it will make upcoming changes
nicer.

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
 src/qemu/qemu_hotplug.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index afb720fc0b..c883fef0e9 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -876,10 +876,9 @@ qemuDomainFindOrCreateSCSIDiskController(virDomainObj *vm,
     cont = g_new0(virDomainControllerDef, 1);
     cont->type = VIR_DOMAIN_CONTROLLER_TYPE_SCSI;
     cont->idx = controller;
-    if (model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT)
-        cont->model = qemuDomainGetSCSIControllerModel(vm->def, cont, priv->qemuCaps);
-    else
-        cont->model = model;
+    cont->model = model;
+
+    cont->model = qemuDomainGetSCSIControllerModel(vm->def, cont, priv->qemuCaps);
 
     if (cont->model < 0) {
         VIR_FREE(cont);
-- 
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:35 +0100, Andrea Bolognani wrote:
> Not a massive difference, but it will make upcoming changes
> nicer.
> 
> Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> ---
>  src/qemu/qemu_hotplug.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index afb720fc0b..c883fef0e9 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -876,10 +876,9 @@ qemuDomainFindOrCreateSCSIDiskController(virDomainObj *vm,
>      cont = g_new0(virDomainControllerDef, 1);
>      cont->type = VIR_DOMAIN_CONTROLLER_TYPE_SCSI;
>      cont->idx = controller;
> -    if (model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT)
> -        cont->model = qemuDomainGetSCSIControllerModel(vm->def, cont, priv->qemuCaps);
> -    else
> -        cont->model = model;
> +    cont->model = model;
> +
> +    cont->model = qemuDomainGetSCSIControllerModel(vm->def, cont, priv->qemuCaps);

This makes no sense at first look. Commit message doesn't explain it,
nothing in the code explains it. You have to look into
qemuDomainGetSCSIControllerModel to see why it can be done.

I've looked ahead to see how you've refactored it, at which point it
makes sense, but I'm not really persuaded that this can't be part of the
commit that is actually fixing the function to not depend on the value
in the 'model' struct.

IMO this patch makes the code strictly worse, even when it's short
lived.
_______________________________________________
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:07:10PM +0100, Peter Krempa wrote:
> On Wed, Jan 24, 2024 at 20:37:35 +0100, Andrea Bolognani wrote:
> > @@ -876,10 +876,9 @@ qemuDomainFindOrCreateSCSIDiskController(virDomainObj *vm,
> >      cont = g_new0(virDomainControllerDef, 1);
> >      cont->type = VIR_DOMAIN_CONTROLLER_TYPE_SCSI;
> >      cont->idx = controller;
> > -    if (model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT)
> > -        cont->model = qemuDomainGetSCSIControllerModel(vm->def, cont, priv->qemuCaps);
> > -    else
> > -        cont->model = model;
> > +    cont->model = model;
> > +
> > +    cont->model = qemuDomainGetSCSIControllerModel(vm->def, cont, priv->qemuCaps);
>
> This makes no sense at first look. Commit message doesn't explain it,
> nothing in the code explains it. You have to look into
> qemuDomainGetSCSIControllerModel to see why it can be done.
>
> I've looked ahead to see how you've refactored it, at which point it
> makes sense, but I'm not really persuaded that this can't be part of the
> commit that is actually fixing the function to not depend on the value
> in the 'model' struct.
>
> IMO this patch makes the code strictly worse, even when it's short
> lived.

That's why I've justified in the commit message as being something
that would pay off later on ;)

I can roll it into the later patch. I think I had it there initially,
but it seemed that it would make reviewing the change harder. Since
you're the one reviewing, I'm happy to go with your preference.

-- 
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:20:05 -0800, Andrea Bolognani wrote:
> On Thu, Jan 25, 2024 at 01:07:10PM +0100, Peter Krempa wrote:
> > On Wed, Jan 24, 2024 at 20:37:35 +0100, Andrea Bolognani wrote:
> > > @@ -876,10 +876,9 @@ qemuDomainFindOrCreateSCSIDiskController(virDomainObj *vm,
> > >      cont = g_new0(virDomainControllerDef, 1);
> > >      cont->type = VIR_DOMAIN_CONTROLLER_TYPE_SCSI;
> > >      cont->idx = controller;
> > > -    if (model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT)
> > > -        cont->model = qemuDomainGetSCSIControllerModel(vm->def, cont, priv->qemuCaps);
> > > -    else
> > > -        cont->model = model;
> > > +    cont->model = model;
> > > +
> > > +    cont->model = qemuDomainGetSCSIControllerModel(vm->def, cont, priv->qemuCaps);
> >
> > This makes no sense at first look. Commit message doesn't explain it,
> > nothing in the code explains it. You have to look into
> > qemuDomainGetSCSIControllerModel to see why it can be done.
> >
> > I've looked ahead to see how you've refactored it, at which point it
> > makes sense, but I'm not really persuaded that this can't be part of the
> > commit that is actually fixing the function to not depend on the value
> > in the 'model' struct.
> >
> > IMO this patch makes the code strictly worse, even when it's short
> > lived.
> 
> That's why I've justified in the commit message as being something
> that would pay off later on ;)
> 
> I can roll it into the later patch. I think I had it there initially,
> but it seemed that it would make reviewing the change harder. Since
> you're the one reviewing, I'm happy to go with your preference.

Please do so, this doesn't get an approval from me in this form.
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org