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

[Joao Martins <joao.m.martins@oracle.com>]

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?

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)