>-----Original Message-----
>From: Joao Martins <joao.m.mart...@oracle.com>
>Sent: Thursday, June 15, 2023 4:54 PM
>To: Duan, Zhenzhong <zhenzhong.d...@intel.com>
>Cc: alex.william...@redhat.com; c...@redhat.com; qemu-devel@nongnu.org;
>Peng, Chao P <chao.p.p...@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.d...@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.
>
>> 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.
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)
