[PATCH 0/7] Fix return type of some functions
Inspired by Peter's patch: https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/TRQ3G... (since it wasn't merged yet, one hunk in patch 7/7 is the same as Peter's; I'll update it after Peter merges his patches) There are some function which are declared to return an int, but in fact return a boolean: int foo(...) { ... return true; } Worse, some mix ints and bools. With a help of coccinelle I was able to identify some offenders. And either redeclare them to return a bool, or fix those (misleading) return statements. Michal Prívozník (7): storage_backend_rbd.c: Make virStorageBackendRBDSetAllocation() stub report an error nwfilter: Fix return type of virNWFilterCanApplyBasicRules callback qemu_process: Fix return type of qemuDomainHasHotpluggableStartupVcpus() storage_backend_rbd.C: Fix return type of a volStorageBackendRBDUseFastDiff() stub virnetdevvlan: Fix return type of virNetDevVlanEqual() virsh-pool.c: Fix return type of virshBuildPoolXML() src: Fix retval of some functions declared to return an int src/ch/ch_hostdev.c | 8 ++++---- src/nwfilter/nwfilter_ebiptables_driver.c | 2 +- src/nwfilter/nwfilter_tech_driver.h | 2 +- src/qemu/qemu_domain.c | 8 ++++---- src/qemu/qemu_process.c | 2 +- src/storage/storage_backend_rbd.c | 5 +++-- src/util/vircommand.c | 2 +- src/util/virnetdevvlan.c | 2 +- src/util/virnetdevvlan.h | 2 +- tools/virsh-pool.c | 2 +- 10 files changed, 18 insertions(+), 17 deletions(-) -- 2.49.0
From: Michal Privoznik <mprivozn@redhat.com> Inside of storage_backend_rbd.c there are two implementations of virStorageBackendRBDSetAllocation(). One reports an error on failure, so the stub implementation should report an error too. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/storage/storage_backend_rbd.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index 832f48df16..c2dbf3a307 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -515,6 +515,7 @@ virStorageBackendRBDSetAllocation(virStorageVolDef *vol G_GNUC_UNUSED, rbd_image_t *image G_GNUC_UNUSED, rbd_image_info_t *info G_GNUC_UNUSED) { + virReportUnsupportedError(); return false; } #endif -- 2.49.0
On Wed, May 14, 2025 at 16:24:10 +0200, Michal Privoznik via Devel wrote:
From: Michal Privoznik <mprivozn@redhat.com>
Inside of storage_backend_rbd.c there are two implementations of virStorageBackendRBDSetAllocation(). One reports an error on failure, so the stub implementation should report an error too.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/storage/storage_backend_rbd.c | 1 + 1 file changed, 1 insertion(+)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
From: Michal Privoznik <mprivozn@redhat.com> The virNWFilterCanApplyBasicRules() callback returns an int but in fact its return type is a boolean. Even its only implementation (ebiptablesCanApplyBasicRules()) returns a boolean. Switch the return type from integer to boolean. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/nwfilter/nwfilter_ebiptables_driver.c | 2 +- src/nwfilter/nwfilter_tech_driver.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c b/src/nwfilter/nwfilter_ebiptables_driver.c index 5082b62577..067df6e612 100644 --- a/src/nwfilter/nwfilter_ebiptables_driver.c +++ b/src/nwfilter/nwfilter_ebiptables_driver.c @@ -2797,7 +2797,7 @@ ebtablesRenameTmpSubAndRootChainsFW(virFirewall *fw, * run ebtablesApplyBasicRules and ebtablesApplyDHCPOnlyRules. * In case of this driver we need the ebtables tool available. */ -static int +static bool ebiptablesCanApplyBasicRules(void) { return true; diff --git a/src/nwfilter/nwfilter_tech_driver.h b/src/nwfilter/nwfilter_tech_driver.h index 8de9eda947..a4af0bf6d5 100644 --- a/src/nwfilter/nwfilter_tech_driver.h +++ b/src/nwfilter/nwfilter_tech_driver.h @@ -51,7 +51,7 @@ typedef int (*virNWFilterRuleTeardownOldRules)(const char *ifname); typedef int (*virNWFilterRuleAllTeardown)(const char *ifname); -typedef int (*virNWFilterCanApplyBasicRules)(void); +typedef bool (*virNWFilterCanApplyBasicRules)(void); typedef int (*virNWFilterApplyBasicRules)(const char *ifname, const virMacAddr *macaddr); -- 2.49.0
On Wed, May 14, 2025 at 16:24:11 +0200, Michal Privoznik via Devel wrote:
From: Michal Privoznik <mprivozn@redhat.com>
The virNWFilterCanApplyBasicRules() callback returns an int but in fact its return type is a boolean. Even its only implementation (ebiptablesCanApplyBasicRules()) returns a boolean. Switch the return type from integer to boolean.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/nwfilter/nwfilter_ebiptables_driver.c | 2 +- src/nwfilter/nwfilter_tech_driver.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
From: Michal Privoznik <mprivozn@redhat.com> The qemuDomainHasHotpluggableStartupVcpus() function is declared to return an int but in fact its return type is a boolean. Even its only caller (qemuProcessLaunch()) threads its retval as a boolean. Switch the return type from integer to boolean. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_process.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 1af91c5909..7fe49adfb4 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -6405,7 +6405,7 @@ qemuProcessValidateHotpluggableVcpus(virDomainDef *def) } -static int +static bool qemuDomainHasHotpluggableStartupVcpus(virDomainDef *def) { size_t maxvcpus = virDomainDefGetVcpusMax(def); -- 2.49.0
On Wed, May 14, 2025 at 16:24:12 +0200, Michal Privoznik via Devel wrote:
From: Michal Privoznik <mprivozn@redhat.com>
The qemuDomainHasHotpluggableStartupVcpus() function is declared to return an int but in fact its return type is a boolean. Even its only caller (qemuProcessLaunch()) threads its retval as a
s/threads/treats/
boolean. Switch the return type from integer to boolean.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_process.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 1af91c5909..7fe49adfb4 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -6405,7 +6405,7 @@ qemuProcessValidateHotpluggableVcpus(virDomainDef *def) }
-static int +static bool qemuDomainHasHotpluggableStartupVcpus(virDomainDef *def) { size_t maxvcpus = virDomainDefGetVcpusMax(def); -- 2.49.0
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
From: Michal Privoznik <mprivozn@redhat.com> Inside of storage_backend.c there are two implementations of volStorageBackendRBDUseFastDiff() function: one when librbd is new enough and one when it isn't. The former returns a bool, but the latter is declared to return an int despite it returning a boolean. Fix the latter. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/storage/storage_backend_rbd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index c2dbf3a307..fd46c8be55 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -503,7 +503,7 @@ volStorageBackendRBDGetFlags(rbd_image_t image G_GNUC_UNUSED, return 0; } -static int +static bool volStorageBackendRBDUseFastDiff(uint64_t features G_GNUC_UNUSED, uint64_t feature_flags G_GNUC_UNUSED) { -- 2.49.0
On Wed, May 14, 2025 at 16:24:13 +0200, Michal Privoznik via Devel wrote:
From: Michal Privoznik <mprivozn@redhat.com>
Inside of storage_backend.c there are two implementations of volStorageBackendRBDUseFastDiff() function: one when librbd is new enough and one when it isn't. The former returns a bool, but the latter is declared to return an int despite it returning a boolean. Fix the latter.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/storage/storage_backend_rbd.c | 2 +-
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
From: Michal Privoznik <mprivozn@redhat.com> The virNetDevVlanEqual() function is declared to return an int but in fact its return type is a boolean. Even its only caller (qemuDomainChangeNet()) threads its retval as a boolean. Switch the return type from integer to boolean. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virnetdevvlan.c | 2 +- src/util/virnetdevvlan.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/util/virnetdevvlan.c b/src/util/virnetdevvlan.c index b0e05d8ffe..453b8d0ac9 100644 --- a/src/util/virnetdevvlan.c +++ b/src/util/virnetdevvlan.c @@ -41,7 +41,7 @@ virNetDevVlanFree(virNetDevVlan *vlan) g_free(vlan); } -int +bool virNetDevVlanEqual(const virNetDevVlan *a, const virNetDevVlan *b) { int ai, bi; diff --git a/src/util/virnetdevvlan.h b/src/util/virnetdevvlan.h index fd2f8023f5..59f7fa523d 100644 --- a/src/util/virnetdevvlan.h +++ b/src/util/virnetdevvlan.h @@ -41,7 +41,7 @@ struct _virNetDevVlan { void virNetDevVlanClear(virNetDevVlan *vlan); void virNetDevVlanFree(virNetDevVlan *vlan); -int virNetDevVlanEqual(const virNetDevVlan *a, const virNetDevVlan *b); +bool virNetDevVlanEqual(const virNetDevVlan *a, const virNetDevVlan *b); void virNetDevVlanCopy(virNetDevVlan *dst, const virNetDevVlan *src); G_DEFINE_AUTOPTR_CLEANUP_FUNC(virNetDevVlan, virNetDevVlanFree); -- 2.49.0
On Wed, May 14, 2025 at 16:24:14 +0200, Michal Privoznik via Devel wrote:
From: Michal Privoznik <mprivozn@redhat.com>
The virNetDevVlanEqual() function is declared to return an int but in fact its return type is a boolean. Even its only caller (qemuDomainChangeNet()) threads its retval as a boolean. Switch
treats
the return type from integer to boolean.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virnetdevvlan.c | 2 +- src/util/virnetdevvlan.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
From: Michal Privoznik <mprivozn@redhat.com> The virshBuildPoolXML() function is declared to return an int but in fact its return type is a boolean. Even its both callers (cmdPoolCreateAs() and cmdPoolDefineAs()) thread its retval as a boolean. Switch the return type from integer to boolean. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/virsh-pool.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/virsh-pool.c b/tools/virsh-pool.c index 33b130564e..089fde55e2 100644 --- a/tools/virsh-pool.c +++ b/tools/virsh-pool.c @@ -322,7 +322,7 @@ static const vshCmdOptDef opts_pool_define_as[] = { {.name = NULL} }; -static int +static bool virshBuildPoolXML(vshControl *ctl, const vshCmd *cmd, const char **retname, -- 2.49.0
On Wed, May 14, 2025 at 16:24:15 +0200, Michal Privoznik via Devel wrote:
From: Michal Privoznik <mprivozn@redhat.com>
The virshBuildPoolXML() function is declared to return an int but in fact its return type is a boolean. Even its both callers (cmdPoolCreateAs() and cmdPoolDefineAs()) thread its retval as a
treat
boolean. Switch the return type from integer to boolean.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/virsh-pool.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/virsh-pool.c b/tools/virsh-pool.c index 33b130564e..089fde55e2 100644 --- a/tools/virsh-pool.c +++ b/tools/virsh-pool.c @@ -322,7 +322,7 @@ static const vshCmdOptDef opts_pool_define_as[] = { {.name = NULL} };
-static int +static bool virshBuildPoolXML(vshControl *ctl, const vshCmd *cmd, const char **retname, -- 2.49.0
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
From: Michal Privoznik <mprivozn@redhat.com> There are couple of functions (virCHDomainPrepareHostdevPCI(), qemuDomainPrepareHostdevPCI(), 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@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 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; } 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 diff --git a/src/util/vircommand.c b/src/util/vircommand.c index ea52acfbb8..d9e4c0181f 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -397,7 +397,7 @@ virCommandHandshakeChild(virCommand *cmd) int rv; if (!cmd->handshake) - return true; + return 0; VIR_DEBUG("Notifying parent for handshake start on %d", cmd->handshakeWait[1]); -- 2.49.0
On Wed, May 14, 2025 at 16:24:16 +0200, Michal Privoznik via Devel wrote:
From: Michal Privoznik <mprivozn@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@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
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@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@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
On Thu, May 15, 2025 at 10:12:46 +0200, Michal Prívozník wrote:
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@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@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.
Ah okay so the updated value is actually used which I missed. It looked weird to me because it copied all the error messages, some of which don't make sense in the context of the 'ch' driver. E.g why mention legacy cpu hotplug when it never supported it?
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.
I thought that the updated value was never used in the first place so that the code would be completely useless. Since it isn't and I don't care about the ch driver that much ...
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().
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
participants (3)
-
Michal Privoznik -
Michal Prívozník
-
Peter Krempa