Re: [PATCH 7/7] src: Fix retval of some functions declared to return an int

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&gt; >> >> 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&gt; >> --- >> 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().