Hello Eric,
On 6/17/24 1:31 PM, Eric Auger wrote:
Hi Cédric,
On 6/17/24 08:33, Cédric Le Goater wrote:
Since vfio_devices_dma_logging_start() takes an 'Error **' argument,
best practices suggest to return a bool. See the api/error.h Rules
section. It will simplify potential changes coming after.
As I already mentionned the Rules section does not say that, as far as I
understand:
It is allowed to either return a bool, a pointer-value, an integer,
along with an error handle:
"
* • bool-valued functions return true on success / false on failure,
* • pointer-valued functions return non-null / null pointer, and
* • integer-valued functions return non-negative / negative.
"
Personally I don't like much returning a bool as I think it rather
complexifies the code and to me that kind of change is error prone.
Returning an int could be misleading too, since there are multiple ways
it could be interpreted. You can find in QEMU routines returning -1 for
error which is later used as an errno :/
The error framework in QEMU provides a way to to save and return any
kind of errors, using the error_seg_*() routines. I tend to prefer
the basic approach: return fail or pass and use the Error parameter
for details.
Now, the problem, as always, is doing the conversion in all the
code. This is probably why people, with a kernel background, find
it confusing.
vfio_container_set_dirty_page_tracking() could be modified in the same
way but the errno value can be saved in the migration stream when
called from vfio_listener_log_global_stop().
Signed-off-by: Cédric Le Goater <c...@redhat.com>
Reviewed-by: Zhenzhong Duan <zhenzhong.d...@intel.com>
---
hw/vfio/common.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index
9e4c0cc95ff90209d3e8184035af0806a2bf890b..d48cd9b9361a92d184e423ffc60aabaff40fb487
100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1020,7 +1020,7 @@ static void vfio_device_feature_dma_logging_start_destroy(
g_free(feature);
}
-static int vfio_devices_dma_logging_start(VFIOContainerBase *bcontainer,
+static bool vfio_devices_dma_logging_start(VFIOContainerBase *bcontainer,
Error **errp)
{
struct vfio_device_feature *feature;
@@ -1033,7 +1033,7 @@ static int
vfio_devices_dma_logging_start(VFIOContainerBase *bcontainer,
&ranges);
if (!feature) {
error_setg_errno(errp, errno, "Failed to prepare DMA logging");
- return -errno;
+ return false;
}
QLIST_FOREACH(vbasedev, &bcontainer->device_list, container_next) {
@@ -1058,7 +1058,7 @@ out:
vfio_device_feature_dma_logging_start_destroy(feature);
- return ret;
+ return ret == 0;
}
static bool vfio_listener_log_global_start(MemoryListener *listener,
@@ -1067,18 +1067,18 @@ static bool
vfio_listener_log_global_start(MemoryListener *listener,
ERRP_GUARD();
VFIOContainerBase *bcontainer = container_of(listener, VFIOContainerBase,
listener);
- int ret;
+ bool ret;
if (vfio_devices_all_device_dirty_tracking(bcontainer)) {
ret = vfio_devices_dma_logging_start(bcontainer, errp);
} else {
- ret = vfio_container_set_dirty_page_tracking(bcontainer, true, errp);
+ ret = vfio_container_set_dirty_page_tracking(bcontainer, true, errp)
== 0;
why vfio_container_set_dirty_page_tracking(bcontainer, true, errp) doesn't
return a bool then?
The errno value can be saved in the migration stream when called from
vfio_listener_log_global_stop(). So this would require changes in the
migration subsystem, like we did for vfio_listener_log_global_start().
Thanks,
C.
Eric
}
- if (ret) {
+ if (!ret) {
error_prepend(errp, "vfio: Could not start dirty page tracking - ");
}
- return !ret;
+ return ret;
}
static void vfio_listener_log_global_stop(MemoryListener *listener)
