| Message ID | 20240617063409.34393-2-clg@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | vfio: QOMify VFIOContainer | expand |
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. > > 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 <clg@redhat.com> > Reviewed-by: Zhenzhong Duan <zhenzhong.duan@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? 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)
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 <clg@redhat.com> >> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@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) >
Hi Cédric, On 6/17/24 14:34, Cédric Le Goater wrote: > 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 :/ yes no good! > > 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 <clg@redhat.com> >>> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@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(). OK so although I am not a big fan of that kind of change and once highlighted this is not mandated by the error.h doc, please feel free to add my Reviewed-by: Eric Auger <eric.auger@redhat.com> Thanks Eric > > 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) >> >
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; } - 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)