Re: [PATCH v2 01/17] vfio: Make vfio_devices_dma_logging_start() return bool

[Eric Auger <eric.auger@redhat.com>]

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)