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

[Duan, Zhenzhong]

>-----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);
    }

Thanks
Zhenzhong
>
>Still it feels strange to use the error pointer to check for that, it's better 
>to
>return error appropriately.
>
>>>
>>>>          return ret;
>>>>      }
>>>>
>>>>      ret = vfio_block_giommu_migration(errp);
>>>> -    if (ret) {
>>>> +    if (ret || (errp && *errp)) {
>>>
>>> Same here?
>>
>> To avoid calling into trace_vfio_migration_probe() which hints vfio migration
>realize succeed.
>>
>
>That was clear, my question was more related to second clause you are
>adding..
>
>> Thanks
>> Zhenzhong
>>
>>>
>>>>          return ret;
>>>>      }
>>>>
>>>> @@ -667,7 +667,7 @@ add_blocker:
>>>>          error_free(vbasedev->migration_blocker);
>>>>          vbasedev->migration_blocker = NULL;
>>>>      }
>>>> -    return ret;
>>>> +    return !ret;
>>>>  }
>>>>
>>>>  void vfio_migration_exit(VFIODevice *vbasedev)