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

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.