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
>