Re: [PATCH 08/14] vfio: Use new Error** argument in vfio_save_setup()

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

On 2/12/24 10:17, Avihai Horon wrote:
> Hi Cedric,
> 
> On 07/02/2024 15:33, Cédric Le Goater wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> Add an Error** argument to vfio_migration_set_state() and adjust
>> callers, including vfio_save_setup(). The error will be propagated up
>> to qemu_savevm_state_setup() where the save_setup() handler is
>> executed.
>>
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>> ---
>>   hw/vfio/migration.c | 62 +++++++++++++++++++++++++++++----------------
>>   1 file changed, 40 insertions(+), 22 deletions(-)
>>
>> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
>> index 2dfbe671f6f45aa530c7341177bb532d8292cecd..2e0a79967cc97f44d9be5575c3cfe18c9f349dab 100644
>> --- a/hw/vfio/migration.c
>> +++ b/hw/vfio/migration.c
>> @@ -84,7 +84,8 @@ static const char *mig_state_to_str(enum vfio_device_mig_state state)
>>
>>   static int vfio_migration_set_state(VFIODevice *vbasedev,
>>                                       enum vfio_device_mig_state new_state,
>> -                                    enum vfio_device_mig_state recover_state)
>> +                                    enum vfio_device_mig_state recover_state,
>> +                                    Error **errp)
>>   {
>>       VFIOMigration *migration = vbasedev->migration;
>>       uint64_t buf[DIV_ROUND_UP(sizeof(struct vfio_device_feature) +
>> @@ -104,15 +105,15 @@ static int vfio_migration_set_state(VFIODevice *vbasedev,
>>           ret = -errno;
>>
>>           if (recover_state == VFIO_DEVICE_STATE_ERROR) {
>> -            error_report("%s: Failed setting device state to %s, err: %s. "
>> -                         "Recover state is ERROR. Resetting device",
>> -                         vbasedev->name, mig_state_to_str(new_state),
>> -                         strerror(errno));
>> +            error_setg(errp, "%s: Failed setting device state to %s, err: %s. "
>> +                       "Recover state is ERROR. Resetting device",
>> +                       vbasedev->name, mig_state_to_str(new_state),
>> +                       strerror(errno));
>>
>>               goto reset_device;
>>           }
>>
>> -        error_report(
>> +        error_setg(errp,
>>               "%s: Failed setting device state to %s, err: %s. Setting device in recover state %s",
>>                        vbasedev->name, mig_state_to_str(new_state),
>>                        strerror(errno), mig_state_to_str(recover_state));
>> @@ -120,7 +121,7 @@ static int vfio_migration_set_state(VFIODevice *vbasedev,
>>           mig_state->device_state = recover_state;
>>           if (ioctl(vbasedev->fd, VFIO_DEVICE_FEATURE, feature)) {
>>               ret = -errno;
>> -            error_report(
>> +            error_setg(errp,
>>                   "%s: Failed setting device in recover state, err: %s. Resetting device",
>>                            vbasedev->name, strerror(errno));
> 
> I think here we will assert because errp is already set.
> 
> Adding an error_append() API would be useful here I guess.

yes.

> Otherwise, we need to move the first error_setg() below, to before we return from a successful recover state change, and construct the error message differently (e.g., provide a full error message for the recover state fail case containing also the first error).
> 
> Do you have other ideas?

Errors for :

     if (ioctl(vbasedev->fd, VFIO_DEVICE_RESET)) {

should be treated as the others with and error_append() and not
hw_error(). This needs a rework before any new changes.

I also wonder why we have twice :

     migration->device_state = recover_state;

It looks redundant. The ioctl VFIO_DEVICE_FEATURE should leave the
state unmodified.

Thanks,

C.