On Wed, Feb 14, 2024 at 18:11:11 +0100, Andrea Bolognani wrote:
> Make the helper stateless. This requires the caller to check
> whether it needs to be called in the first place instead of
> adding this check inside the function, which makes for more
> readable, if a little more verbose, code.
>
> At the same time, change it so that it uses an out argument to
> return the selected model back to the caller, and only use the
> return value to signal whether the function completed its task
> successfully. This is consistent with most APIs in libvirt, and
> removes the ambiguity caused by the fact that the value of
> VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT is -1.
>
> In order to have a nice API while still keeping the
> implementation short and readable, a small wrapper is required.
Well arguably the wrapper is not needed unless you really want to use:
return VIR_DOMAIN_CONTROLLER_MODEL_SCS*;
rather than:
*model = VIR_DOMAIN_CONTROLLER_MODEL_SCS*;
return 0;
>
> Signed-off-by: Andrea Bolognani <abologna@xxxxxxxxxx>
> ---
> src/qemu/qemu_domain.c | 51 ++++++++++++++++++++++++-----------------
> src/qemu/qemu_domain.h | 4 ++--
> src/qemu/qemu_hotplug.c | 18 ++++++++-------
> 3 files changed, 42 insertions(+), 31 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 22e6b9fd3e..ab2cc427a2 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -4096,6 +4096,26 @@ qemuDomainDefAddDefaultAudioBackend(virQEMUDriver *driver,
> }
>
>
> +/* Internal. Use qemuDomainDefaultSCSIControllerModel() instead */
> +static virDomainControllerModelSCSI
> +qemuDomainDefaultSCSIControllerModelInternal(const virDomainDef *def,
> + virQEMUCaps *qemuCaps)
> +{
> + if (qemuDomainIsPSeries(def))
> + return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI;
> + if (ARCH_IS_S390(def->os.arch))
> + return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI;
> + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SCSI_LSI))
> + return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC;
> + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_SCSI))
> + return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI;
> + if (qemuDomainHasBuiltinESP(def))
> + return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_NCR53C90;
> +
> + return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT;
> +}
> +
> +
> /**
> * @def: Domain definition
> * @cont: Domain controller def
> @@ -4107,25 +4127,12 @@ qemuDomainDefAddDefaultAudioBackend(virQEMUDriver *driver,
> * Returns model on success, -1 on failure with error set.
So now this always returns 0 -> success, thus the return value is
pointless. Further up the next patch which modifies the documentation
still keeps the claim. Additionally the description will read:
Not being able to come up with a value is NOT considered a failure.
which will mean that this function will never fail.
IMO it would be sufficient to just declare:
virDomainControllerModelSCSI
qemuDomainDefaultSCSIControllerModel(const virDomainDef *def,
const virDomainControllerDef *cont,
...
and just document the same claims. Thus if _DEFAULT is returned it's up
to the caller to decide.
Fabricating a return value doesn't seem to make much sense for me and
once you only return success you might as well as return the value
directly. It'd simplify the checker conditions.
_______________________________________________
Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
