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é <phi...@linaro.org> Signed-off-by: Cédric Le Goater <c...@redhat.com> --- Changes in v4: - Dropped log_global_stop() and log_global_sync() changesChanges 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
