Re: Re: [PATCH 17/33] qemu: Rename qemuDomainDefaultSCSIControllerModel()
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