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

[Cédric Le Goater]

On 3/7/24 09:15, Eric Auger wrote:
> Hi Cédric,
>
> On 3/6/24 14:34, Cédric Le Goater wrote:
> > This allows to update the Error argument of the VFIO log_global_start()
> > handler. Errors detected when device level logging is started will be
> > propagated up to qemu_savevm_state_setup() when the ram save_setup()
> > handler is executed.
> >
> > The vfio_set_migration_error() call becomes redundant. Remove it.
> you may precise it becomes redundant in vfio_listener_log_global_start()
> because it is kept in vfio_listener_log_global_stop

Yes. This is a leftover from v3 which still had the changes for
the .log_global_stop() handlers.


> > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > Signed-off-by: Cédric Le Goater <clg@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_start()
> >   - ERRP_GUARD() because of error_prepend use in
> >     vfio_listener_log_global_start()
> >     
> >   hw/vfio/common.c | 25 ++++++++++++++-----------
> >   1 file changed, 14 insertions(+), 11 deletions(-)
> >
> > diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> > index 
> > 5598a508399a6c0b3a20ba17311cbe83d84250c5..d6790557da2f2890398fa03dbbef18129cd2c1bb
> >  100644
> > --- a/hw/vfio/common.c
> > +++ b/hw/vfio/common.c
> > @@ -1036,7 +1036,8 @@ static void vfio_device_feature_dma_logging_start_destroy(
> >       g_free(feature);
> >   }
> >   
> > -static int vfio_devices_dma_logging_start(VFIOContainerBase *bcontainer)
> > +static int vfio_devices_dma_logging_start(VFIOContainerBase *bcontainer,
> > +                                          Error **errp)
> >   {
> >       struct vfio_device_feature *feature;
> >       VFIODirtyRanges ranges;
> > @@ -1058,8 +1059,8 @@ static int 
> > vfio_devices_dma_logging_start(VFIOContainerBase *bcontainer)
> >           ret = ioctl(vbasedev->fd, VFIO_DEVICE_FEATURE, feature);
> >           if (ret) {
> >               ret = -errno;
> there is another case of error if !feature. Don't we want t o set errp
> in that case as well?

arf. How did I miss that ... Will fix.

> I think in general we should try to make the return value and the errp
> consistent because the caller may try to exploit the errp in case or
> negative returned value.

yes. That's the goal.

Thanks,

C.


> > -            error_report("%s: Failed to start DMA logging, err %d (%s)",
> > -                         vbasedev->name, ret, strerror(errno));
> > +            error_setg_errno(errp, errno, "%s: Failed to start DMA logging",
> > +                             vbasedev->name);
> >               goto out;
> >           }
> >           vbasedev->dirty_tracking = true;
> > @@ -1078,20 +1079,19 @@ out:
> >   static bool vfio_listener_log_global_start(MemoryListener *listener,
> >                                              Error **errp)
> >   {
> > +    ERRP_GUARD(); /* error_prepend use */
> >       VFIOContainerBase *bcontainer = container_of(listener, VFIOContainerBase,
> >                                                    listener);
> >       int ret;
> >   
> >       if (vfio_devices_all_device_dirty_tracking(bcontainer)) {
> > -        ret = vfio_devices_dma_logging_start(bcontainer);
> > +        ret = vfio_devices_dma_logging_start(bcontainer, errp);
> >       } else {
> > -        ret = vfio_container_set_dirty_page_tracking(bcontainer, true, NULL);
> > +        ret = vfio_container_set_dirty_page_tracking(bcontainer, true, errp);
> >       }
> >   
> >       if (ret) {
> > -        error_report("vfio: Could not start dirty page tracking, err: %d (%s)",
> > -                     ret, strerror(-ret));
> > -        vfio_set_migration_error(ret);
> > +        error_prepend(errp, "vfio: Could not start dirty page tracking - ");
> >       }
> >       return !ret;
> >   }
> > @@ -1100,17 +1100,20 @@ static void 
> > vfio_listener_log_global_stop(MemoryListener *listener)
> >   {
> >       VFIOContainerBase *bcontainer = container_of(listener, VFIOContainerBase,
> >                                                    listener);
> > +    Error *local_err = NULL;
> >       int ret = 0;
> >   
> >       if (vfio_devices_all_device_dirty_tracking(bcontainer)) {
> >           vfio_devices_dma_logging_stop(bcontainer);
> >       } else {
> > -        ret = vfio_container_set_dirty_page_tracking(bcontainer, false, NULL);
> > +        ret = vfio_container_set_dirty_page_tracking(bcontainer, false,
> > +                                                     &local_err);
> >       }
> >   
> >       if (ret) {
> > -        error_report("vfio: Could not stop dirty page tracking, err: %d (%s)",
> > -                     ret, strerror(-ret));
> > +        error_prepend(&local_err,
> > +                      "vfio: Could not stop dirty page tracking - ");
> > +        error_report_err(local_err);
> >           vfio_set_migration_error(ret);
> >       }
> >   }
> Eric