Include:
vfio_block_multiple_devices_migration(),
vfio_block_giommu_migration(),
vfio_block_migration(),
vfio_migration_realize().

Suggested-by: Cédric Le Goater <clg@redhat.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 hw/vfio/common.c              | 16 ++++++++--------
 hw/vfio/migration.c           | 19 ++++++++++++-------
 include/hw/vfio/vfio-common.h |  6 +++---
 3 files changed, 23 insertions(+), 18 deletions(-)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 77e2ee0e5c6e..00fef5aa08be 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -381,19 +381,19 @@ static unsigned int vfio_migratable_device_num(void)
     return device_num;
 }
 
-int vfio_block_multiple_devices_migration(VFIODevice *vbasedev, Error **errp)
+bool vfio_block_multiple_devices_migration(VFIODevice *vbasedev, Error **errp)
 {
     int ret;
 
     if (multiple_devices_migration_blocker ||
         vfio_migratable_device_num() <= 1) {
-        return 0;
+        return true;
     }
 
     if (vbasedev->enable_migration == ON_OFF_AUTO_ON) {
         error_setg(errp, "Migration is currently not supported with multiple "
                          "VFIO devices");
-        return -EINVAL;
+        return false;
     }
 
     error_setg(&multiple_devices_migration_blocker,
@@ -405,7 +405,7 @@ int vfio_block_multiple_devices_migration(VFIODevice *vbasedev, Error **errp)
         multiple_devices_migration_blocker = NULL;
     }
 
-    return ret;
+    return !ret;
 }
 
 void vfio_unblock_multiple_devices_migration(void)
@@ -433,19 +433,19 @@ static bool vfio_viommu_preset(void)
     return false;
 }
 
-int vfio_block_giommu_migration(VFIODevice *vbasedev, Error **errp)
+bool vfio_block_giommu_migration(VFIODevice *vbasedev, Error **errp)
 {
     int ret;
 
     if (giommu_migration_blocker ||
         !vfio_viommu_preset()) {
-        return 0;
+        return true;
     }
 
     if (vbasedev->enable_migration == ON_OFF_AUTO_ON) {
         error_setg(errp,
                    "Migration is currently not supported with vIOMMU enabled");
-        return -EINVAL;
+        return false;
     }
 
     error_setg(&giommu_migration_blocker,
@@ -456,7 +456,7 @@ int vfio_block_giommu_migration(VFIODevice *vbasedev, 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 1db7d52ab2c1..09fa4714a085 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -802,13 +802,13 @@ static int vfio_migration_init(VFIODevice *vbasedev)
     return 0;
 }
 
-static int vfio_block_migration(VFIODevice *vbasedev, Error *err, Error **errp)
+static bool vfio_block_migration(VFIODevice *vbasedev, Error *err, Error **errp)
 {
     int ret;
 
     if (vbasedev->enable_migration == ON_OFF_AUTO_ON) {
         error_propagate(errp, err);
-        return -EINVAL;
+        return false;
     }
 
     vbasedev->migration_blocker = error_copy(err);
@@ -820,7 +820,7 @@ static int vfio_block_migration(VFIODevice *vbasedev, Error *err, Error **errp)
         vbasedev->migration_blocker = NULL;
     }
 
-    return ret;
+    return !ret;
 }
 
 /* ---------------------------------------------------------------------- */
@@ -835,7 +835,12 @@ void vfio_reset_bytes_transferred(void)
     bytes_transferred = 0;
 }
 
-int vfio_migration_realize(VFIODevice *vbasedev, Error **errp)
+/*
+ * Return true when either migration initialized or blocker registered.
+ * Currently only return false when adding blocker fails which will
+ * de-register vfio device.
+ */
+bool vfio_migration_realize(VFIODevice *vbasedev, Error **errp)
 {
     Error *err = NULL;
     int ret;
@@ -874,17 +879,17 @@ int vfio_migration_realize(VFIODevice *vbasedev, Error **errp)
     }
 
     ret = vfio_block_multiple_devices_migration(vbasedev, errp);
-    if (ret) {
+    if (!ret) {
         return ret;
     }
 
     ret = vfio_block_giommu_migration(vbasedev, errp);
-    if (ret) {
+    if (!ret) {
         return ret;
     }
 
     trace_vfio_migration_realize(vbasedev->name);
-    return 0;
+    return true;
 }
 
 void vfio_migration_exit(VFIODevice *vbasedev)
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index 93429b9abba0..b3014ece2edf 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -225,9 +225,9 @@ typedef QLIST_HEAD(VFIOGroupList, VFIOGroup) VFIOGroupList;
 extern VFIOGroupList vfio_group_list;
 
 bool vfio_mig_active(void);
-int vfio_block_multiple_devices_migration(VFIODevice *vbasedev, Error **errp);
+bool vfio_block_multiple_devices_migration(VFIODevice *vbasedev, Error **errp);
 void vfio_unblock_multiple_devices_migration(void);
-int vfio_block_giommu_migration(VFIODevice *vbasedev, Error **errp);
+bool vfio_block_giommu_migration(VFIODevice *vbasedev, Error **errp);
 int64_t vfio_mig_bytes_transferred(void);
 void vfio_reset_bytes_transferred(void);
 
@@ -252,7 +252,7 @@ int vfio_spapr_create_window(VFIOContainer *container,
 int vfio_spapr_remove_window(VFIOContainer *container,
                              hwaddr offset_within_address_space);
 
-int vfio_migration_realize(VFIODevice *vbasedev, Error **errp);
+bool vfio_migration_realize(VFIODevice *vbasedev, Error **errp);
 void vfio_migration_exit(VFIODevice *vbasedev);
 void vfio_migration_finalize(void);
 
-- 
2.34.1

On 30/06/2023 08:36, Zhenzhong Duan wrote:
> Include:
> vfio_block_multiple_devices_migration(),
> vfio_block_giommu_migration(),
> vfio_block_migration(),
> vfio_migration_realize().
> 

Maybe some brief commit message would be useful as to why we are renaming it
("Migration disabled" no longer applies as that is being removed):

Make vfio_migration_realize() adhere to the convention of other realize()
callbacks by returning bool instead of int. In doing so, change also the the
functions used in vfio_migration_realize() return path to have bool too.

But see a remark below -- I think if you move the rename patch to be the last
one, you will have less to rename (?) Unless we really want to rename everything
that vfio_migration_realize() ends up calling to return bool. In which case my
comment then no longer applies. I generally favour not loosing error information
until last minute, but OTOH Error is provided so errors will be propagated in
some way.

> Suggested-by: Cédric Le Goater <clg@redhat.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>  hw/vfio/common.c              | 16 ++++++++--------
>  hw/vfio/migration.c           | 19 ++++++++++++-------
>  include/hw/vfio/vfio-common.h |  6 +++---
>  3 files changed, 23 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 77e2ee0e5c6e..00fef5aa08be 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -381,19 +381,19 @@ static unsigned int vfio_migratable_device_num(void)
>      return device_num;
>  }
>  
> -int vfio_block_multiple_devices_migration(VFIODevice *vbasedev, Error **errp)
> +bool vfio_block_multiple_devices_migration(VFIODevice *vbasedev, Error **errp)
>  {
>      int ret;
>  
>      if (multiple_devices_migration_blocker ||
>          vfio_migratable_device_num() <= 1) {
> -        return 0;
> +        return true;
>      }
>  
>      if (vbasedev->enable_migration == ON_OFF_AUTO_ON) {
>          error_setg(errp, "Migration is currently not supported with multiple "
>                           "VFIO devices");
> -        return -EINVAL;
> +        return false;
>      }
>  
>      error_setg(&multiple_devices_migration_blocker,
> @@ -405,7 +405,7 @@ int vfio_block_multiple_devices_migration(VFIODevice *vbasedev, Error **errp)
>          multiple_devices_migration_blocker = NULL;
>      }
>  
> -    return ret;
> +    return !ret;
>  }
>  
>  void vfio_unblock_multiple_devices_migration(void)
> @@ -433,19 +433,19 @@ static bool vfio_viommu_preset(void)
>      return false;
>  }
>  
> -int vfio_block_giommu_migration(VFIODevice *vbasedev, Error **errp)
> +bool vfio_block_giommu_migration(VFIODevice *vbasedev, Error **errp)
>  {
>      int ret;
>  
>      if (giommu_migration_blocker ||
>          !vfio_viommu_preset()) {
> -        return 0;
> +        return true;
>      }
>  
>      if (vbasedev->enable_migration == ON_OFF_AUTO_ON) {
>          error_setg(errp,
>                     "Migration is currently not supported with vIOMMU enabled");
> -        return -EINVAL;
> +        return false;
>      }
>  
>      error_setg(&giommu_migration_blocker,
> @@ -456,7 +456,7 @@ int vfio_block_giommu_migration(VFIODevice *vbasedev, 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 1db7d52ab2c1..09fa4714a085 100644
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c
> @@ -802,13 +802,13 @@ static int vfio_migration_init(VFIODevice *vbasedev)
>      return 0;
>  }
>  
> -static int vfio_block_migration(VFIODevice *vbasedev, Error *err, Error **errp)
> +static bool vfio_block_migration(VFIODevice *vbasedev, Error *err, Error **errp)
>  {
>      int ret;
>  
>      if (vbasedev->enable_migration == ON_OFF_AUTO_ON) {
>          error_propagate(errp, err);
> -        return -EINVAL;
> +        return false;
>      }
>  
>      vbasedev->migration_blocker = error_copy(err);
> @@ -820,7 +820,7 @@ static int vfio_block_migration(VFIODevice *vbasedev, Error *err, Error **errp)
>          vbasedev->migration_blocker = NULL;
>      }
>  
> -    return ret;
> +    return !ret;
>  }
>  
>  /* ---------------------------------------------------------------------- */
> @@ -835,7 +835,12 @@ void vfio_reset_bytes_transferred(void)
>      bytes_transferred = 0;
>  }
> 

With your later patches where you remove vIOMMU migration blocker and have a
common return path in vfio_migration_realize() I suspect you will need to only
rename vfio_migration_realize() and you negate all the values accordingly in
that function, and thus hopefully reducing (...)

> -int vfio_migration_realize(VFIODevice *vbasedev, Error **errp)
> +/*
> + * Return true when either migration initialized or blocker registered.
> + * Currently only return false when adding blocker fails which will
> + * de-register vfio device.
> + */
> +bool vfio_migration_realize(VFIODevice *vbasedev, Error **errp)
>  {
>      Error *err = NULL;
>      int ret;
> @@ -874,17 +879,17 @@ int vfio_migration_realize(VFIODevice *vbasedev, Error **errp)
>      }
>  
>      ret = vfio_block_multiple_devices_migration(vbasedev, errp);
> -    if (ret) {
> +    if (!ret) {
>          return ret;
>      }
>  
>      ret = vfio_block_giommu_migration(vbasedev, errp);
> -    if (ret) {
> +    if (!ret) {
>          return ret;
>      }
>  
>      trace_vfio_migration_realize(vbasedev->name);
> -    return 0;
> +    return true;
>  }
>  

(...) to just this one last chunk?

>  void vfio_migration_exit(VFIODevice *vbasedev)
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index 93429b9abba0..b3014ece2edf 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -225,9 +225,9 @@ typedef QLIST_HEAD(VFIOGroupList, VFIOGroup) VFIOGroupList;
>  extern VFIOGroupList vfio_group_list;
>  
>  bool vfio_mig_active(void);
> -int vfio_block_multiple_devices_migration(VFIODevice *vbasedev, Error **errp);
> +bool vfio_block_multiple_devices_migration(VFIODevice *vbasedev, Error **errp);
>  void vfio_unblock_multiple_devices_migration(void);
> -int vfio_block_giommu_migration(VFIODevice *vbasedev, Error **errp);
> +bool vfio_block_giommu_migration(VFIODevice *vbasedev, Error **errp);
>  int64_t vfio_mig_bytes_transferred(void);
>  void vfio_reset_bytes_transferred(void);
>  
> @@ -252,7 +252,7 @@ int vfio_spapr_create_window(VFIOContainer *container,
>  int vfio_spapr_remove_window(VFIOContainer *container,
>                               hwaddr offset_within_address_space);
>  
> -int vfio_migration_realize(VFIODevice *vbasedev, Error **errp);
> +bool vfio_migration_realize(VFIODevice *vbasedev, Error **errp);
>  void vfio_migration_exit(VFIODevice *vbasedev);
>  void vfio_migration_finalize(void);
>  

>-----Original Message-----
>From: Joao Martins <joao.m.martins@oracle.com>
>Sent: Friday, June 30, 2023 6:41 PM
>Subject: Re: [PATCH v5 4/7] vfio/migration: Return bool type for some vfio
>migration related functions
>
>On 30/06/2023 08:36, Zhenzhong Duan wrote:
>> Include:
>> vfio_block_multiple_devices_migration(),
>> vfio_block_giommu_migration(),
>> vfio_block_migration(),
>> vfio_migration_realize().
>>
>
>Maybe some brief commit message would be useful as to why we are
>renaming it
>("Migration disabled" no longer applies as that is being removed):
>
>Make vfio_migration_realize() adhere to the convention of other realize()
>callbacks by returning bool instead of int. In doing so, change also the the
>functions used in vfio_migration_realize() return path to have bool too.
>
>But see a remark below -- I think if you move the rename patch to be the last
>one, you will have less to rename (?) Unless we really want to rename
>everything
>that vfio_migration_realize() ends up calling to return bool. In which case my
>comment then no longer applies. I generally favour not loosing error
>information
>until last minute, but OTOH Error is provided so errors will be propagated in
>some way.
Make sense, I'll only rename vfio_migration_realize() and for other
internal functions, keep their error info in return value as you suggested.
This way we still meet Cedric's expectation of vfio_migration_realize()
following the qdev_realize() prototype.

Thanks
Zhenzhong