On Wed, May 14, 2025 at 16:24:16 +0200, Michal Privoznik via Devel wrote:
> From: Michal Privoznik <mpriv...@redhat.com>
>
> There are couple of functions (virCHDomainPrepareHostdevPCI(),
> qemuDomainPrepareHostdevPCI(),
I've pushed my patch so this will likely drop from your list.
> virStorageBackendRBDSetAllocation(), virCommandHandshakeChild())
> that are declared to return an integer, but in fact return a
> boolean. This may lead to incorrect behaviour. Fix their retvals.
>
> This diff was generated using the following semantic patch:
>
> @@
> identifier foo;
> @@
>
> int foo(...) {
> <+...
> (
> - return true;
> + return 0;
> |
> - return false;
> + return -1;
> )
> ...+>
> }
>
> Each function and its callers were then inspected to see what
> retvals are expected.
>
> Signed-off-by: Michal Privoznik <mpriv...@redhat.com>
> ---
> src/ch/ch_hostdev.c | 8 ++++----
> src/qemu/qemu_domain.c | 8 ++++----
> src/storage/storage_backend_rbd.c | 2 +-
> src/util/vircommand.c | 2 +-
> 4 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/src/ch/ch_hostdev.c b/src/ch/ch_hostdev.c
> index 641032de30..34eb025c97 100644
> --- a/src/ch/ch_hostdev.c
> +++ b/src/ch/ch_hostdev.c
> @@ -69,20 +69,20 @@ virCHDomainPrepareHostdevPCI(virDomainHostdevDef *hostdev)
> if (!supportsPassthroughVFIO) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("host doesn't support VFIO PCI passthrough"));
> - return false;
> + return -1;
> }
> break;
>
> case VIR_DEVICE_HOSTDEV_PCI_DRIVER_NAME_KVM:
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("host doesn't support legacy PCI passthrough"));
> - return false;
> + return -1;
>
> case VIR_DEVICE_HOSTDEV_PCI_DRIVER_NAME_XEN:
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> _("CH does not support device assignment mode
> '%1$s'"),
>
> virDeviceHostdevPCIDriverNameTypeToString(*driverName));
> - return false;
> + return -1;
>
> default:
> case VIR_DEVICE_HOSTDEV_PCI_DRIVER_NAME_LAST:
> @@ -90,7 +90,7 @@ virCHDomainPrepareHostdevPCI(virDomainHostdevDef *hostdev)
> break;
> }
>
> - return true;
> + return 0;
> }
>
> int
The presence of this copy of this function made me suspicious and I
actually wasn't able to find any code that'd use the the result of this
function (e.g. the driver name it sets).
Since you have your R-b and S-o on the patch adding the copy you might
know?
Anyways since the cloud hypervisor code doesn't use the result and you
are making few code paths fatal, won't this break the hotplug in the CH
driver?
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 52da234343..7a34bc1c7f 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -9945,20 +9945,20 @@ qemuDomainPrepareHostdevPCI(virDomainHostdevDef
> *hostdev,
> if (!supportsPassthroughVFIO) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("host doesn't support VFIO PCI passthrough"));
> - return false;
> + return -1;
> }
> break;
>
> case VIR_DEVICE_HOSTDEV_PCI_DRIVER_NAME_KVM:
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("host doesn't support legacy PCI passthrough"));
> - return false;
> + return -1;
>
> case VIR_DEVICE_HOSTDEV_PCI_DRIVER_NAME_XEN:
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> _("QEMU does not support device assignment mode
> '%1$s'"),
>
> virDeviceHostdevPCIDriverNameTypeToString(*driverName));
> - return false;
> + return -1;
>
> default:
> case VIR_DEVICE_HOSTDEV_PCI_DRIVER_NAME_LAST:
> @@ -9966,7 +9966,7 @@ qemuDomainPrepareHostdevPCI(virDomainHostdevDef
> *hostdev,
> break;
> }
>
> - return true;
> + return 0;
> }
>
>
Both hunks in this file should vanish after rebase/merge
> diff --git a/src/storage/storage_backend_rbd.c
> b/src/storage/storage_backend_rbd.c
> index fd46c8be55..038a1a9e34 100644
> --- a/src/storage/storage_backend_rbd.c
> +++ b/src/storage/storage_backend_rbd.c
> @@ -516,7 +516,7 @@ virStorageBackendRBDSetAllocation(virStorageVolDef *vol
> G_GNUC_UNUSED,
> rbd_image_info_t *info G_GNUC_UNUSED)
> {
> virReportUnsupportedError();
> - return false;
> + return -1;
> }
> #endif
>
This hunk looks good
