Re: [PATCH] vfio/migration: Fix return value of vfio_migration_realize()

[Joao Martins]

On 16/06/2023 03:42, Duan, Zhenzhong wrote:
>> -----Original Message-----
>> From: Joao Martins <joao.m.martins@oracle.com>
>> Sent: Thursday, June 15, 2023 6:23 PM
>> To: Duan, Zhenzhong <zhenzhong.duan@intel.com>
>> Cc: alex.williamson@redhat.com; clg@redhat.com; qemu-devel@nongnu.org;
>> Peng, Chao P <chao.p.peng@intel.com>
>> Subject: Re: [PATCH] vfio/migration: Fix return value of 
>> vfio_migration_realize()
>>
>> On 15/06/2023 10:19, Duan, Zhenzhong wrote:
>>>> -----Original Message-----
>>>> From: Joao Martins <joao.m.martins@oracle.com>
>>>> Sent: Thursday, June 15, 2023 4:54 PM
>>>> To: Duan, Zhenzhong <zhenzhong.duan@intel.com>
>>>> Cc: alex.williamson@redhat.com; clg@redhat.com;
>>>> qemu-devel@nongnu.org; Peng, Chao P <chao.p.peng@intel.com>
>>>> Subject: Re: [PATCH] vfio/migration: Fix return value of
>>>> vfio_migration_realize()
>>>>
>>>>
>>>>
>>>> On 15/06/2023 09:18, Zhenzhong Duan wrote:
>>>>> We should print "Migration disabled" when migration is blocked in
>>>>> vfio_migration_realize().
>>>>>
>>>>> Fix it by reverting return value of migrate_add_blocker(), meanwhile
>>>>> error out directly once migrate_add_blocker() failed.
>>>>>
>>>>
>>>> It wasn't immediately obvious from commit message that this is mainly
>>>> about just printing an error message when blockers get added and that
>>>> well
>>>> migrate_add_blocker() returns 0 on success and caller of
>>>> vfio_migration_realize expects the opposite when blockers are added.
>>>>
>>>> Perhaps better wording would be:
>>>>
>>>> migrate_add_blocker() returns 0 when successfully adding the
>>>> migration blocker. However, the caller of vfio_migration_realize()
>>>> considers that migration was blocked when the latter returned an
>>>> error. Fix it by negating the return value obtained by
>>>> migrate_add_blocker(). What matters for migration is that the blocker
>>>> is added in core migration, so this cleans up usability such that
>>>> user sees "Migrate disabled" when any of the vfio migration blockers are
>> active.
>>>
>>> Great, I'll use your words.
>>>
>>>>
>>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>>>> ---
>>>>>  hw/vfio/common.c    | 4 ++--
>>>>>  hw/vfio/migration.c | 6 +++---
>>>>>  2 files changed, 5 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c index
>>>>> fa8fd949b1cf..8505385798f3 100644
>>>>> --- a/hw/vfio/common.c
>>>>> +++ b/hw/vfio/common.c
>>>>> @@ -399,7 +399,7 @@ int vfio_block_multiple_devices_migration(Error
>>>> **errp)
>>>>>          multiple_devices_migration_blocker = NULL;
>>>>>      }
>>>>>
>>>>> -    return ret;
>>>>> +    return !ret;
>>>>>  }
>>>>>
>>>>>  void vfio_unblock_multiple_devices_migration(void)
>>>>> @@ -444,7 +444,7 @@ int vfio_block_giommu_migration(Error **errp)
>>>>>          giommu_migration_blocker = NULL;
>>>>>      }
>>>>>
>>>>> -    return ret;
>>>>> +    return !ret;
>>>>>  }
>>>>>
>>>>>  void vfio_migration_finalize(void)
>>>>> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c index
>>>>> 6b58dddb8859..0146521d129a 100644
>>>>> --- a/hw/vfio/migration.c
>>>>> +++ b/hw/vfio/migration.c
>>>>> @@ -646,12 +646,12 @@ int vfio_migration_realize(VFIODevice
>>>>> *vbasedev,
>>>> Error **errp)
>>>>>      }
>>>>>
>>>>>      ret = vfio_block_multiple_devices_migration(errp);
>>>>> -    if (ret) {
>>>>> +    if (ret || (errp && *errp)) {
>>>>
>>>> Why do you need this extra clause?
>>>
>>> Now that error happens, no need to add other blockers which will fail
>>> for same reason.
>>>
>>
>> But you don't need the (errp && *errp) for that as that's the whole point of
>> negating the result.
>>
>> And if there's an error set it means migrate_add_blocker failed to add the
>> blocker (e.g. snapshotting IIUC), and you would be failing here 
>> unnecessarily?
> 
> If there is an error qdev_device_add() will fail, continue execution is 
> meaningless here?
> There is ERRP_GUARD in this path, so it looks (*errp) is enough.
> 
> If I removed (errp && *errp) to continue, need below change to bypass 
> trace_vfio_migration_probe
> Do you prefer this way?
> 
>     if (!*errp) {
>         trace_vfio_migration_probe(vbasedev->name);
>     }
>

I am mainly questioning that the error testing is correct to test here.

IIUC, the only one that can propagate any *new* error in vfio_migration_realize
is the calls to migrate_add_blocker failing within the vfio_block* (migration
code suggests that this happens on snapshotting). Failing to add migration
blocker just means we haven't installed any blockers. And the current code
presents that as a "Migration disabled" case. If we want to preserve that
behaviour on migration_add_blocker() failures (which seems like that's what you
are doing here) then instead of this:

        return !ret;

you would do this:

        ret = migration_add_blocker(...);
        if (ret < 0) {
                error_free(...);
                blocker = NULL;
+               return ret;
        }

+       return 1;

Or something like that. And then if you return early as you intended?