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 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.

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