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