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