Re: [PATCH v4 20/25] vfio: Add Error** argument to .vfio_save_config() handler

[Eric Auger]

On 3/6/24 14:34, Cédric Le Goater wrote:
> Use vmstate_save_state_with_err() to improve error reporting in the
> callers and store a reported error under the migration stream. Add
> documentation while at it.
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>  include/hw/vfio/vfio-common.h | 25 ++++++++++++++++++++++++-
>  hw/vfio/migration.c           | 18 ++++++++++++------
>  hw/vfio/pci.c                 |  5 +++--
>  3 files changed, 39 insertions(+), 9 deletions(-)
> 
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index 
> b9da6c08ef41174610eb92726c590309a53696a3..46f88493634b5634a9c14a5caa33a463fbf2c50d
>  100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -133,7 +133,30 @@ struct VFIODeviceOps {
>      int (*vfio_hot_reset_multi)(VFIODevice *vdev);
>      void (*vfio_eoi)(VFIODevice *vdev);
>      Object *(*vfio_get_object)(VFIODevice *vdev);
> -    void (*vfio_save_config)(VFIODevice *vdev, QEMUFile *f);
> +
> +    /**
> +     * @vfio_save_config
> +     *
> +     * Save device config state
> +     *
> +     * @vdev: #VFIODevice for which to save the config
> +     * @f: #QEMUFile where to send the data
> +     * @errp: pointer to Error*, to store an error if it happens.
> +     *
> +     * Returns zero to indicate success and negative for error
> +     */
> +    int (*vfio_save_config)(VFIODevice *vdev, QEMUFile *f, Error **errp);
> +
> +    /**
> +     * @vfio_load_config
> +     *
> +     * Load device config state
> +     *
> +     * @vdev: #VFIODevice for which to load the config
> +     * @f: #QEMUFile where to get the data
> +     *
> +     * Returns zero to indicate success and negative for error
> +     */
>      int (*vfio_load_config)(VFIODevice *vdev, QEMUFile *f);
>  };
>  
> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> index 
> 71ade14a7942358094371a86c00718f5979113ea..bd48f2ee472a5230c2c84bff829dae1e217db33f
>  100644
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c
> @@ -190,14 +190,19 @@ static int vfio_load_buffer(QEMUFile *f, VFIODevice 
> *vbasedev,
>      return ret;
>  }
>  
> -static int vfio_save_device_config_state(QEMUFile *f, void *opaque)
> +static int vfio_save_device_config_state(QEMUFile *f, void *opaque,
> +                                         Error **errp)
>  {
>      VFIODevice *vbasedev = opaque;
> +    int ret;
>  
>      qemu_put_be64(f, VFIO_MIG_FLAG_DEV_CONFIG_STATE);
>  
>      if (vbasedev->ops && vbasedev->ops->vfio_save_config) {
> -        vbasedev->ops->vfio_save_config(vbasedev, f);
> +        ret = vbasedev->ops->vfio_save_config(vbasedev, f, errp);
> +        if (ret) {
I am not familiar enough with that case but don't you still want to set
the VFIO_MIG_FLAG_END_OF_STATE to "close" the state?

Eric
> +            return ret;
> +        }
>      }
>  
>      qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE);
> @@ -587,13 +592,14 @@ static int vfio_save_complete_precopy(QEMUFile *f, void 
> *opaque)
>  static void vfio_save_state(QEMUFile *f, void *opaque)
>  {
>      VFIODevice *vbasedev = opaque;
> +    Error *local_err = NULL;
>      int ret;
>  
> -    ret = vfio_save_device_config_state(f, opaque);
> +    ret = vfio_save_device_config_state(f, opaque, &local_err);
>      if (ret) {
> -        error_report("%s: Failed to save device config space",
> -                     vbasedev->name);
> -        qemu_file_set_error(f, ret);
> +        error_prepend(&local_err, "%s: Failed to save device config space",
> +                      vbasedev->name);
> +        qemu_file_set_error_obj(f, ret, local_err);
>      }
>  }
>  
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 
> 4fa387f0430d62ca2ba1b5ae5b7037f8f06b33f9..99d86e1d40ef25133fc76ad6e58294b07bd20843
>  100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -2585,11 +2585,12 @@ const VMStateDescription vmstate_vfio_pci_config = {
>      }
>  };
>  
> -static void vfio_pci_save_config(VFIODevice *vbasedev, QEMUFile *f)
> +static int vfio_pci_save_config(VFIODevice *vbasedev, QEMUFile *f, Error 
> **errp)
>  {
>      VFIOPCIDevice *vdev = container_of(vbasedev, VFIOPCIDevice, vbasedev);
>  
> -    vmstate_save_state(f, &vmstate_vfio_pci_config, vdev, NULL);
> +    return vmstate_save_state_with_err(f, &vmstate_vfio_pci_config, vdev, 
> NULL,
> +                                       errp);
>  }
>  
>  static int vfio_pci_load_config(VFIODevice *vbasedev, QEMUFile *f)