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

[Avihai Horon <avihaih@nvidia.com>]

On 12/02/2024 19:54, Cédric Le Goater wrote:
> External email: Use caution opening links or attachments
>
>
> 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;

Not sure where you see we have it twice.

One time we set "mig_state->device_state = recover_state;" for the ioctl.
And another time we set "migration->device_state = recover_state;" in 
case of successful ioctl, to update the device state in migration struct.

>
> It looks redundant. The ioctl VFIO_DEVICE_FEATURE should leave the
> state unmodified.
>
> Thanks,
>
> C.
>
>