Re: [PATCH v4 18/25] vfio: Add Error** argument to vfio_devices_dma_logging_stop()

[Eric Auger]

On 3/6/24 14:34, Cédric Le Goater wrote:
> This improves error reporting in the log_global_stop() VFIO handler.
> 
> Reviewed-by: Philippe Mathieu-Daudé <phi...@linaro.org>
> Signed-off-by: Cédric Le Goater <c...@redhat.com>
> ---
> 
>  Changes in v4:
> 
>  - Dropped log_global_stop() and log_global_sync() changes
>    
>  Changes in v3:
> 
>  - Use error_setg_errno() in vfio_devices_dma_logging_stop() 
>  
>  hw/vfio/common.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 
> d6790557da2f2890398fa03dbbef18129cd2c1bb..5b2e6a179cdd5f8ca5be84b7097661e96b391456
>  100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -938,12 +938,14 @@ static void vfio_dirty_tracking_init(VFIOContainerBase 
> *bcontainer,
>      memory_listener_unregister(&dirty.listener);
>  }
>  
> -static void vfio_devices_dma_logging_stop(VFIOContainerBase *bcontainer)
> +static int vfio_devices_dma_logging_stop(VFIOContainerBase *bcontainer,
> +                                          Error **errp)
>  {
>      uint64_t buf[DIV_ROUND_UP(sizeof(struct vfio_device_feature),
>                                sizeof(uint64_t))] = {};
>      struct vfio_device_feature *feature = (struct vfio_device_feature *)buf;
>      VFIODevice *vbasedev;
> +    int ret = 0;
>  
>      feature->argsz = sizeof(buf);
>      feature->flags = VFIO_DEVICE_FEATURE_SET |
> @@ -955,11 +957,17 @@ static void 
> vfio_devices_dma_logging_stop(VFIOContainerBase *bcontainer)
>          }
>  
>          if (ioctl(vbasedev->fd, VFIO_DEVICE_FEATURE, feature)) {
> -            warn_report("%s: Failed to stop DMA logging, err %d (%s)",
> -                        vbasedev->name, -errno, strerror(errno));
> +            /* Keep first error */
> +            if (!ret) {
> +                ret = -errno;
> +                error_setg_errno(errp, errno, "%s: Failed to stop DMA 
> logging",
> +                                 vbasedev->name);
maybe you can keep the previous warn_report in case errp is NULL
(rollback) or for subsequent failures?

Eric
> +            }
>          }
>          vbasedev->dirty_tracking = false;
>      }
> +
> +    return ret;
>  }
>  
>  static struct vfio_device_feature *
> @@ -1068,7 +1076,8 @@ static int 
> vfio_devices_dma_logging_start(VFIOContainerBase *bcontainer,
>  
>  out:
>      if (ret) {
> -        vfio_devices_dma_logging_stop(bcontainer);
> +        /* Ignore the potential errors when doing rollback */
> +        vfio_devices_dma_logging_stop(bcontainer, NULL);
>      }
>  
>      vfio_device_feature_dma_logging_start_destroy(feature);
> @@ -1104,7 +1113,7 @@ static void 
> vfio_listener_log_global_stop(MemoryListener *listener)
>      int ret = 0;
>  
>      if (vfio_devices_all_device_dirty_tracking(bcontainer)) {
> -        vfio_devices_dma_logging_stop(bcontainer);
> +        ret = vfio_devices_dma_logging_stop(bcontainer, &local_err);
>      } else {
>          ret = vfio_container_set_dirty_page_tracking(bcontainer, false,
>                                                       &local_err);