3:12 a.m.
On 5/14/25 16:57, Peter Krempa wrote:
On Wed, May 14, 2025 at 16:24:16 +0200, Michal Privoznik via Devel
wrote:
> From: Michal Privoznik <mprivozn(a)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 <mprivozn(a)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?
I'm not sure I follow.
Firstly, the pci driver used is formatted into <hostdev/> (see
virDeviceHostdevPCIDriverInfoFormat()).
Secondly, this function merely checks whether VFIO passthrough is
available in the host AND domain XML has either driver='vfio' or no
driver at all (in which case it is set to VFIO). All other drivers
report an error; just like qemuDomainPrepareHostdevPCI() would do.
Now, in CH you can't really compile VFIO support out, thus it doesn't
make sense to check for caps (and we don't have that implemented for CH
driver anyways, yet).
Thirdly, the pci driver is used in virCHHostdevPrepareDomainDevices() ->
virHostdevPreparePCIDevices() when it uses the (now possibly changed)
driver to detach a PCI device from the host system.
And finally, the retval of virCHDomainPrepareHostdevPCI() is checked for
by its caller(s): virCHDomainPrepareHostdev() ->
virCHProcessPrepareDomainHostdevs() -> virCHProcessPrepareDomain() ->
virCHProcessStart()
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?
This code was buggy to begin with. Exactly for the same reason you are
fixing in your patch. CH does not support any other type of PCI
assignment than VFIO. Therefore, one could argue that requesting say
driver='xen' in the domain XML was buggy and should have never worked.
> 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
And they had, indeed.
> 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
And what about the other one in
src/util/vircommand.c:virCommandHandshakeChild().
Michal