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 10: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",
>> +                             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).

Yes this is like qemu_fflush(). I will also change the test to

   if (ret < 0)

As Prasad suggested.


Thanks,

C.