Re: [PATCH v3 02/26] vfio: Always report an error in vfio_save_setup()

["Cédric Le Goater" <clg@redhat.com>]

On 3/6/24 11:16, Avihai Horon wrote:
> 
> On 06/03/2024 11:56, Avihai Horon wrote:
>>
>> On 04/03/2024 14:28, Cédric Le Goater wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> This will prepare ground for futur changes adding an Error** argument
>>> to the save_setup() handler. We need to make sure that on failure,
>>> vfio_save_setup() always sets a new error.
>>>
>>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>>> ---
>>>   hw/vfio/migration.c | 15 ++++++++++++---
>>>   1 file changed, 12 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
>>> index 2050ac8897231ff89cc223f0570d5c7a65dede9e..51bea536cc290ba0aa393f78b017b0650e333bff 100644
>>> --- a/hw/vfio/migration.c
>>> +++ b/hw/vfio/migration.c
>>> @@ -383,6 +383,7 @@ static int vfio_save_setup(QEMUFile *f, void *opaque)
>>>       VFIODevice *vbasedev = opaque;
>>>       VFIOMigration *migration = vbasedev->migration;
>>>       uint64_t stop_copy_size = VFIO_MIG_DEFAULT_DATA_BUFFER_SIZE;
>>> +    int ret;
>>>
>>>       qemu_put_be64(f, VFIO_MIG_FLAG_DEV_SETUP_STATE);
>>>
>>> @@ -397,13 +398,13 @@ static int vfio_save_setup(QEMUFile *f, void *opaque)
>>>       }
>>>
>>>       if (vfio_precopy_supported(vbasedev)) {
>>> -        int ret;
>>> -
>>>           switch (migration->device_state) {
>>>           case VFIO_DEVICE_STATE_RUNNING:
>>>               ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_PRE_COPY,
>>> VFIO_DEVICE_STATE_RUNNING);
>>>               if (ret) {
>>> +                error_report("%s: Failed to set new RUNNING state",
> 
> Oh, sorry, forgot to mention in previous mail:
> s/RUNNING/PRE_COPY

Ah yes. Parameters are <new> and <recover> state.

Thanks,

C.




> 
>>> + vbasedev->name);
>>>                   return ret;
>>>               }
>>>
>>> @@ -414,6 +415,8 @@ static int vfio_save_setup(QEMUFile *f, void *opaque)
>>>               /* vfio_save_complete_precopy() will go to STOP_COPY */
>>>               break;
>>>           default:
>>> +            error_report("%s: Invalid device state %d", vbasedev->name,
>>> +                         migration->device_state);
>>>               return -EINVAL;
>>>           }
>>>       }
>>> @@ -422,7 +425,13 @@ static int vfio_save_setup(QEMUFile *f, void *opaque)
>>>
>>>       qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE);
>>>
>>> -    return qemu_file_get_error(f);
>>> +    ret = qemu_file_get_error(f);
>>> +    if (ret) {
>>> +        error_report("%s: save setup failed : %s", vbasedev->name,
>>> +                     strerror(ret));
>>
>> Here it should be -ret (and also later in patch #12).
>>
>> Thanks.
>>
>>> +    }
>>> +
>>> +    return ret;
>>>   }
>>>
>>>   static void vfio_save_cleanup(void *opaque)
>>> -- 
>>> 2.44.0
>>>
>