Summary states just "Rename"
On Wed, Jan 24, 2024 at 20:37:37 +0100, Andrea Bolognani wrote:
> The function is modified to be stateless, which is more
> consistent with existing helpers that deal with figuring out
> default models for devices, and its name needs to change
> accordingly.
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'
> Signed-off-by: Andrea Bolognani <abologna@xxxxxxxxxx>
> ---
> src/qemu/qemu_domain.c | 21 +++++++++++----------
> src/qemu/qemu_domain.h | 6 +++---
> src/qemu/qemu_hotplug.c | 5 +++--
> 3 files changed, 17 insertions(+), 15 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 672f1ce59f..97336d5002 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -4106,23 +4106,23 @@ qemuDomainDefAddDefaultAudioBackend(virQEMUDriver *driver,
>
>
> /**
> + * qemuDomainDefaultSCSIControllerModel:
> * @def: Domain definition
> * @cont: Domain controller def
> * @qemuCaps: qemu capabilities
> *
> - * If the controller model is already defined, return it immediately;
> - * otherwise, based on the @qemuCaps return a default model value.
> + * Decides the preferred model for a SCSI controller that it to be
> + * added to @def. The choice might be based on various factors,
> + * including the architecture, machine type and what devices are
> + * available according to @qemuCaps.
> *
> * Returns model on success, -1 on failure with error set.
> */
> int
> -qemuDomainGetSCSIControllerModel(const virDomainDef *def,
> - const virDomainControllerDef *cont,
> - virQEMUCaps *qemuCaps)
> +qemuDomainDefaultSCSIControllerModel(const virDomainDef *def,
> + const virDomainControllerDef *cont,
> + virQEMUCaps *qemuCaps)
> {
> - if (cont->model > 0)
> - return cont->model;
> -
> if (qemuDomainIsPSeries(def))
> return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI;
> if (ARCH_IS_S390(def->os.arch))
> @@ -5621,9 +5621,10 @@ qemuDomainControllerDefPostParse(virDomainControllerDef *cont,
> switch (cont->type) {
> 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.
Also this still is sub-optimal as
VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT is a valid enum member with
value of -1;
Obviously the preferrable solution would be to return the model via
pointer/argument and just return success error from the main function to
avoid the problems described above.
With the commit message fixed and one of:
- remove the 'cont->model <= 0' changes
- the comment describing the return values improved
Reviewed-by: Peter Krempa <pkrempa@xxxxxxxxxx>
_______________________________________________
Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
