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

[Joao Martins]

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.

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

>          return ret;
>      }
>  
>      ret = vfio_block_giommu_migration(errp);
> -    if (ret) {
> +    if (ret || (errp && *errp)) {

Same here?

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