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.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
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.
*/
int
-qemuDomainDefaultSCSIControllerModel(const virDomainDef *def,
- const virDomainControllerDef *cont,
+qemuDomainDefaultSCSIControllerModel(virDomainControllerModelSCSI *model,
+ const virDomainDef *def,
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))
- 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 -1;
+ *model = qemuDomainDefaultSCSIControllerModelInternal(def, qemuCaps);
+ return 0;
}
@@ -5610,10 +5617,12 @@ qemuDomainControllerDefPostParse(virDomainControllerDef *cont,
{
switch (cont->type) {
case VIR_DOMAIN_CONTROLLER_TYPE_SCSI:
- /* Set the default SCSI controller model if not already set */
- cont->model = qemuDomainDefaultSCSIControllerModel(def, cont, qemuCaps);
-
- if (cont->model < 0) {
+ /* If no model is set, try to come up with a reasonable
+ * default. If one can't be determined, error out */
+ if ((cont->model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT ||
+ cont->model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_AUTO) &&
+ (qemuDomainDefaultSCSIControllerModel(&cont->model, def, qemuCaps) < 0 ||
+ cont->model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT)) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Unable to determine model for SCSI controller idx=%1$d"),
cont->idx);
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index f3aad2ef5c..dae817d655 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -838,8 +838,8 @@ bool qemuDomainHasBuiltinESP(const virDomainDef *def);
bool qemuDomainNeedsFDC(const virDomainDef *def);
bool qemuDomainSupportsPCI(const virDomainDef *def);
bool qemuDomainSupportsPCIMultibus(const virDomainDef *def);
-int qemuDomainDefaultSCSIControllerModel(const virDomainDef *def,
- const virDomainControllerDef *cont,
+int qemuDomainDefaultSCSIControllerModel(virDomainControllerModelSCSI *model,
+ const virDomainDef *def,
virQEMUCaps *qemuCaps);
void qemuDomainUpdateCurrentMemorySize(virDomainObj *vm);
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index a9dbc37915..b96c706804 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -850,7 +850,7 @@ qemuDomainFindOrCreateSCSIDiskController(virDomainObj *vm,
size_t i;
virDomainControllerDef *cont;
qemuDomainObjPrivate *priv = vm->privateData;
- int model = -1;
+ virDomainControllerModelSCSI model = VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT;
for (i = 0; i < vm->def->ncontrollers; i++) {
cont = vm->def->controllers[i];
@@ -876,13 +876,15 @@ qemuDomainFindOrCreateSCSIDiskController(virDomainObj *vm,
* now hotplug a controller */
cont = virDomainControllerDefNew(VIR_DOMAIN_CONTROLLER_TYPE_SCSI);
cont->idx = controller;
-
- if (model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT)
- cont->model = qemuDomainDefaultSCSIControllerModel(vm->def, cont, priv->qemuCaps);
- else
- cont->model = model;
-
- if (cont->model < 0) {
+ cont->model = model;
+
+ /* If no model has been discovered by looking at existing SCSI
+ * controllers, try to come up with a reasonable default. If one
+ * can't be determined, error out */
+ if ((cont->model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT ||
+ cont->model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_AUTO) &&
+ (qemuDomainDefaultSCSIControllerModel(&cont->model, vm->def, priv->qemuCaps) < 0 ||
+ cont->model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT)) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Unable to determine model for SCSI controller idx=%1$d"),
cont->idx);
--
2.43.0
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
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@redhat.com>
> ---
> 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@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
On Thu, Feb 15, 2024 at 01:57:27PM +0100, Peter Krempa wrote: > On Wed, Feb 14, 2024 at 18:11:11 +0100, Andrea Bolognani wrote: > > 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; I really do want to be able to use the former style. It doesn't make as big an impact here, but for the USB part the difference is quite stark. > 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. You're right, I can absolutely get rid of the wrapper. Failure states that needed to be reported actually existed at some point, but those went away as I reworked the code, and I never went back to reconsider the API. Thanks a lot for the suggestion, and consider it done :) -- Andrea Bolognani / Red Hat / Virtualization _______________________________________________ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-leave@lists.libvirt.org
On Thu, Feb 15, 2024 at 05:46:28 -0800, Andrea Bolognani wrote: > On Thu, Feb 15, 2024 at 01:57:27PM +0100, Peter Krempa wrote: > > On Wed, Feb 14, 2024 at 18:11:11 +0100, Andrea Bolognani wrote: > > > 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; > > I really do want to be able to use the former style. It doesn't make > as big an impact here, but for the USB part the difference is quite > stark. > > > 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. > > You're right, I can absolutely get rid of the wrapper. > > Failure states that needed to be reported actually existed at some > point, but those went away as I reworked the code, and I never went > back to reconsider the API. > > Thanks a lot for the suggestion, and consider it done :) You can use: Reviewed-by: Peter Krempa <pkrempa@redhat.com> _______________________________________________ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-leave@lists.libvirt.org
© 2016 - 2026 Red Hat, Inc.