Re: Re: [PATCH 15/33] qemu: Simplify qemuDomainFindOrCreateSCSIDiskController()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux