[PATCH 0/3] migration: Cleanup around MigMode sets

[PATCH 0/3] migration: Cleanup around MigMode sets

[Markus Armbruster]

Markus Armbruster (3):
  migration: Use unsigned instead of int for bit set of MigMode
  migration: Use bitset of MigMode instead of variable arguments
  migration: Put Error **errp parameter last

 include/migration/blocker.h |  9 ++----
 include/migration/misc.h    | 10 +++----
 hw/vfio/container-legacy.c  |  6 ++--
 hw/vfio/cpr-iommufd.c       |  6 ++--
 hw/vfio/cpr-legacy.c        |  8 +++---
 hw/vfio/cpr.c               |  5 ++--
 hw/vfio/device.c            |  4 +--
 migration/migration.c       | 57 +++++++++----------------------------
 stubs/migr-blocker.c        |  2 +-
 system/physmem.c            |  8 +++---
 10 files changed, 40 insertions(+), 75 deletions(-)

-- 
2.49.0

[PATCH 1/3] migration: Use unsigned instead of int for bit set of MigMode

[Markus Armbruster]

Signed operands in bitwise operations are unwise.  I believe they're
safe here, but avoiding them is easy, so let's do that.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 migration/migration.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index a63b46bbef..9f0a91b1d9 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1672,9 +1672,9 @@ void migration_cancel(void)
     }
 }
 
-static int get_modes(MigMode mode, va_list ap);
+static unsigned get_modes(MigMode mode, va_list ap);
 
-static void add_notifiers(NotifierWithReturn *notify, int modes)
+static void add_notifiers(NotifierWithReturn *notify, unsigned modes)
 {
     for (MigMode mode = 0; mode < MIG_MODE__MAX; mode++) {
         if (modes & BIT(mode)) {
@@ -1687,7 +1687,7 @@ static void add_notifiers(NotifierWithReturn *notify, int modes)
 void migration_add_notifier_modes(NotifierWithReturn *notify,
                                   MigrationNotifyFunc func, MigMode mode, ...)
 {
-    int modes;
+    unsigned modes;
     va_list ap;
 
     va_start(ap, mode);
@@ -1876,7 +1876,7 @@ static bool is_busy(Error **reasonp, Error **errp)
     return false;
 }
 
-static bool is_only_migratable(Error **reasonp, Error **errp, int modes)
+static bool is_only_migratable(Error **reasonp, Error **errp, unsigned modes)
 {
     ERRP_GUARD();
 
@@ -1890,9 +1890,9 @@ static bool is_only_migratable(Error **reasonp, Error **errp, int modes)
     return false;
 }
 
-static int get_modes(MigMode mode, va_list ap)
+static unsigned get_modes(MigMode mode, va_list ap)
 {
-    int modes = 0;
+    unsigned modes = 0;
 
     while (mode != -1 && mode != MIG_MODE_ALL) {
         assert(mode >= MIG_MODE_NORMAL && mode < MIG_MODE__MAX);
@@ -1905,7 +1905,7 @@ static int get_modes(MigMode mode, va_list ap)
     return modes;
 }
 
-static int add_blockers(Error **reasonp, Error **errp, int modes)
+static int add_blockers(Error **reasonp, Error **errp, unsigned modes)
 {
     for (MigMode mode = 0; mode < MIG_MODE__MAX; mode++) {
         if (modes & BIT(mode)) {
@@ -1928,7 +1928,7 @@ int migrate_add_blocker_normal(Error **reasonp, Error **errp)
 
 int migrate_add_blocker_modes(Error **reasonp, Error **errp, MigMode mode, ...)
 {
-    int modes;
+    unsigned modes;
     va_list ap;
 
     va_start(ap, mode);
@@ -1945,7 +1945,7 @@ int migrate_add_blocker_modes(Error **reasonp, Error **errp, MigMode mode, ...)
 
 int migrate_add_blocker_internal(Error **reasonp, Error **errp)
 {
-    int modes = BIT(MIG_MODE__MAX) - 1;
+    unsigned modes = BIT(MIG_MODE__MAX) - 1;
 
     if (is_busy(reasonp, errp)) {
         return -EBUSY;
-- 
2.49.0

[PATCH 2/3] migration: Use bitset of MigMode instead of variable arguments

[Markus Armbruster]

migrate_add_blocker_modes() and migration_add_notifier_modes use
variable arguments for a set of migration modes.  The variable
arguments get collected into a bitset for processsing.  Take a bitset
argument instead, it's simpler.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/migration/blocker.h |  9 +++-----
 include/migration/misc.h    | 10 ++++-----
 hw/vfio/container-legacy.c  |  6 +++---
 hw/vfio/cpr-iommufd.c       |  6 +++---
 hw/vfio/cpr-legacy.c        |  8 +++----
 hw/vfio/cpr.c               |  5 ++---
 hw/vfio/device.c            |  4 ++--
 migration/migration.c       | 43 ++++++-------------------------------
 stubs/migr-blocker.c        |  2 +-
 system/physmem.c            |  8 +++----
 10 files changed, 33 insertions(+), 68 deletions(-)

diff --git a/include/migration/blocker.h b/include/migration/blocker.h
index a687ac0efe..80b75ad5cb 100644
--- a/include/migration/blocker.h
+++ b/include/migration/blocker.h
@@ -16,8 +16,6 @@
 
 #include "qapi/qapi-types-migration.h"
 
-#define MIG_MODE_ALL MIG_MODE__MAX
-
 /**
  * @migrate_add_blocker - prevent all modes of migration from proceeding
  *
@@ -82,16 +80,15 @@ int migrate_add_blocker_normal(Error **reasonp, Error **errp);
  *
  * @reasonp - address of an error to be returned whenever migration is attempted
  *
- * @errp - [out] The reason (if any) we cannot block migration right now.
+ * @modes - the migration modes to be blocked, a bit set of MigMode
  *
- * @mode - one or more migration modes to be blocked.  The list is terminated
- *         by -1 or MIG_MODE_ALL.  For the latter, all modes are blocked.
+ * @errp - [out] The reason (if any) we cannot block migration right now.
  *
  * @returns - 0 on success, -EBUSY/-EACCES on failure, with errp set.
  *
  * *@reasonp is freed and set to NULL if failure is returned.
  * On success, the caller must not free *@reasonp before the blocker is removed.
  */
-int migrate_add_blocker_modes(Error **reasonp, Error **errp, MigMode mode, ...);
+int migrate_add_blocker_modes(Error **reasonp, unsigned modes, Error **errp);
 
 #endif
diff --git a/include/migration/misc.h b/include/migration/misc.h
index 592b93021e..e26d418a6e 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -90,18 +90,18 @@ void migration_add_notifier(NotifierWithReturn *notify,
                             MigrationNotifyFunc func);
 
 /*
- * Same as migration_add_notifier, but applies to be specified @mode.
+ * Same as migration_add_notifier, but applies to the specified @mode
+ * instead of MIG_MODE_NORMAL.
  */
 void migration_add_notifier_mode(NotifierWithReturn *notify,
                                  MigrationNotifyFunc func, MigMode mode);
 
 /*
- * Same as migration_add_notifier, but applies to all @mode in the argument
- * list.  The list is terminated by -1 or MIG_MODE_ALL.  For the latter,
- * the notifier is added for all modes.
+ * Same as migration_add_notifier, but applies to the specified @modes
+ * (a bitset of MigMode).
  */
 void migration_add_notifier_modes(NotifierWithReturn *notify,
-                                  MigrationNotifyFunc func, MigMode mode, ...);
+                                  MigrationNotifyFunc func, unsigned modes);
 
 /*
  * Remove a notifier from all modes.
diff --git a/hw/vfio/container-legacy.c b/hw/vfio/container-legacy.c
index 8e9639603e..32c260b345 100644
--- a/hw/vfio/container-legacy.c
+++ b/hw/vfio/container-legacy.c
@@ -977,9 +977,9 @@ static bool vfio_legacy_attach_device(const char *name, VFIODevice *vbasedev,
     if (vbasedev->mdev) {
         error_setg(&vbasedev->cpr.mdev_blocker,
                    "CPR does not support vfio mdev %s", vbasedev->name);
-        if (migrate_add_blocker_modes(&vbasedev->cpr.mdev_blocker, errp,
-                                      MIG_MODE_CPR_TRANSFER, MIG_MODE_CPR_EXEC,
-                                      -1) < 0) {
+        if (migrate_add_blocker_modes(&vbasedev->cpr.mdev_blocker,
+                    BIT(MIG_MODE_CPR_TRANSFER) | BIT(MIG_MODE_CPR_EXEC),
+                    errp) < 0) {
             goto hiod_unref_exit;
         }
     }
diff --git a/hw/vfio/cpr-iommufd.c b/hw/vfio/cpr-iommufd.c
index 8a4d65de5e..c244db88fb 100644
--- a/hw/vfio/cpr-iommufd.c
+++ b/hw/vfio/cpr-iommufd.c
@@ -158,9 +158,9 @@ bool vfio_iommufd_cpr_register_iommufd(IOMMUFDBackend *be, Error **errp)
     Error **cpr_blocker = &be->cpr_blocker;
 
     if (!vfio_cpr_supported(be, cpr_blocker)) {
-        return migrate_add_blocker_modes(cpr_blocker, errp,
-                                         MIG_MODE_CPR_TRANSFER,
-                                         MIG_MODE_CPR_EXEC, -1) == 0;
+        return migrate_add_blocker_modes(cpr_blocker,
+                    BIT(MIG_MODE_CPR_TRANSFER) | BIT(MIG_MODE_CPR_TRANSFER),
+                    errp);
     }
 
     vmstate_register(NULL, -1, &iommufd_cpr_vmstate, be);
diff --git a/hw/vfio/cpr-legacy.c b/hw/vfio/cpr-legacy.c
index 7184c93991..86c943158e 100644
--- a/hw/vfio/cpr-legacy.c
+++ b/hw/vfio/cpr-legacy.c
@@ -176,9 +176,9 @@ bool vfio_legacy_cpr_register_container(VFIOLegacyContainer *container,
                                 MIG_MODE_CPR_REBOOT);
 
     if (!vfio_cpr_supported(container, cpr_blocker)) {
-        return migrate_add_blocker_modes(cpr_blocker, errp,
-                                         MIG_MODE_CPR_TRANSFER,
-                                         MIG_MODE_CPR_EXEC, -1) == 0;
+        return migrate_add_blocker_modes(cpr_blocker,
+                        BIT(MIG_MODE_CPR_TRANSFER) | BIT(MIG_MODE_CPR_EXEC),
+                        errp) == 0;
     }
 
     vfio_cpr_add_kvm_notifier();
@@ -187,7 +187,7 @@ bool vfio_legacy_cpr_register_container(VFIOLegacyContainer *container,
 
     migration_add_notifier_modes(&container->cpr.transfer_notifier,
                                  vfio_cpr_fail_notifier,
-                                 MIG_MODE_CPR_TRANSFER, MIG_MODE_CPR_EXEC, -1);
+                                 BIT(MIG_MODE_CPR_TRANSFER) | BIT(MIG_MODE_CPR_EXEC));
     return true;
 }
 
diff --git a/hw/vfio/cpr.c b/hw/vfio/cpr.c
index db462aabcb..998230d271 100644
--- a/hw/vfio/cpr.c
+++ b/hw/vfio/cpr.c
@@ -197,8 +197,7 @@ void vfio_cpr_add_kvm_notifier(void)
     if (!kvm_close_notifier.notify) {
         migration_add_notifier_modes(&kvm_close_notifier,
                                      vfio_cpr_kvm_close_notifier,
-                                     MIG_MODE_CPR_TRANSFER, MIG_MODE_CPR_EXEC,
-                                     -1);
+                                     BIT(MIG_MODE_CPR_TRANSFER) | BIT(MIG_MODE_CPR_EXEC));
     }
 }
 
@@ -285,7 +284,7 @@ void vfio_cpr_pci_register_device(VFIOPCIDevice *vdev)
 {
     migration_add_notifier_modes(&vdev->cpr.transfer_notifier,
                                  vfio_cpr_pci_notifier,
-                                 MIG_MODE_CPR_TRANSFER, MIG_MODE_CPR_EXEC, -1);
+                                 BIT(MIG_MODE_CPR_TRANSFER) | BIT(MIG_MODE_CPR_EXEC));
 }
 
 void vfio_cpr_pci_unregister_device(VFIOPCIDevice *vdev)
diff --git a/hw/vfio/device.c b/hw/vfio/device.c
index 8b63e765ac..76869828fc 100644
--- a/hw/vfio/device.c
+++ b/hw/vfio/device.c
@@ -345,8 +345,8 @@ bool vfio_device_get_name(VFIODevice *vbasedev, Error **errp)
                            "vfio device with fd=%d needs an id property",
                            vbasedev->fd);
                 return migrate_add_blocker_modes(&vbasedev->cpr.id_blocker,
-                                                 errp, MIG_MODE_CPR_TRANSFER,
-                                                 -1) == 0;
+                                                 BIT(MIG_MODE_CPR_TRANSFER),
+                                                 errp) == 0;
             }
         }
     }
diff --git a/migration/migration.c b/migration/migration.c
index 9f0a91b1d9..5f1c83aafd 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1672,8 +1672,6 @@ void migration_cancel(void)
     }
 }
 
-static unsigned get_modes(MigMode mode, va_list ap);
-
 static void add_notifiers(NotifierWithReturn *notify, unsigned modes)
 {
     for (MigMode mode = 0; mode < MIG_MODE__MAX; mode++) {
@@ -1685,15 +1683,8 @@ static void add_notifiers(NotifierWithReturn *notify, unsigned modes)
 }
 
 void migration_add_notifier_modes(NotifierWithReturn *notify,
-                                  MigrationNotifyFunc func, MigMode mode, ...)
+                                  MigrationNotifyFunc func, unsigned modes)
 {
-    unsigned modes;
-    va_list ap;
-
-    va_start(ap, mode);
-    modes = get_modes(mode, ap);
-    va_end(ap);
-
     notify->notify = (NotifierWithReturnFunc)func;
     add_notifiers(notify, modes);
 }
@@ -1701,13 +1692,13 @@ void migration_add_notifier_modes(NotifierWithReturn *notify,
 void migration_add_notifier_mode(NotifierWithReturn *notify,
                                  MigrationNotifyFunc func, MigMode mode)
 {
-    migration_add_notifier_modes(notify, func, mode, -1);
+    migration_add_notifier_modes(notify, func, BIT(mode));
 }
 
 void migration_add_notifier(NotifierWithReturn *notify,
                             MigrationNotifyFunc func)
 {
-    migration_add_notifier_modes(notify, func, MIG_MODE_NORMAL, -1);
+    migration_add_notifier_mode(notify, func, MIG_MODE_NORMAL);
 }
 
 void migration_remove_notifier(NotifierWithReturn *notify)
@@ -1890,21 +1881,6 @@ static bool is_only_migratable(Error **reasonp, Error **errp, unsigned modes)
     return false;
 }
 
-static unsigned get_modes(MigMode mode, va_list ap)
-{
-    unsigned modes = 0;
-
-    while (mode != -1 && mode != MIG_MODE_ALL) {
-        assert(mode >= MIG_MODE_NORMAL && mode < MIG_MODE__MAX);
-        modes |= BIT(mode);
-        mode = va_arg(ap, MigMode);
-    }
-    if (mode == MIG_MODE_ALL) {
-        modes = BIT(MIG_MODE__MAX) - 1;
-    }
-    return modes;
-}
-
 static int add_blockers(Error **reasonp, Error **errp, unsigned modes)
 {
     for (MigMode mode = 0; mode < MIG_MODE__MAX; mode++) {
@@ -1918,23 +1894,16 @@ static int add_blockers(Error **reasonp, Error **errp, unsigned modes)
 
 int migrate_add_blocker(Error **reasonp, Error **errp)
 {
-    return migrate_add_blocker_modes(reasonp, errp, MIG_MODE_ALL);
+    return migrate_add_blocker_modes(reasonp, -1u, errp);
 }
 
 int migrate_add_blocker_normal(Error **reasonp, Error **errp)
 {
-    return migrate_add_blocker_modes(reasonp, errp, MIG_MODE_NORMAL, -1);
+    return migrate_add_blocker_modes(reasonp, BIT(MIG_MODE_NORMAL), errp);
 }
 
-int migrate_add_blocker_modes(Error **reasonp, Error **errp, MigMode mode, ...)
+int migrate_add_blocker_modes(Error **reasonp, unsigned modes, Error **errp)
 {
-    unsigned modes;
-    va_list ap;
-
-    va_start(ap, mode);
-    modes = get_modes(mode, ap);
-    va_end(ap);
-
     if (is_only_migratable(reasonp, errp, modes)) {
         return -EACCES;
     } else if (is_busy(reasonp, errp)) {
diff --git a/stubs/migr-blocker.c b/stubs/migr-blocker.c
index 11cbff268f..e54c7160d3 100644
--- a/stubs/migr-blocker.c
+++ b/stubs/migr-blocker.c
@@ -11,7 +11,7 @@ int migrate_add_blocker_normal(Error **reasonp, Error **errp)
     return 0;
 }
 
-int migrate_add_blocker_modes(Error **reasonp, Error **errp, MigMode mode, ...)
+int migrate_add_blocker_modes(Error **reasonp, unsigned modes, Error **errp)
 {
     return 0;
 }
diff --git a/system/physmem.c b/system/physmem.c
index a340ca3e61..a7e2a5d07f 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -2255,8 +2255,8 @@ static void ram_block_add(RAMBlock *new_block, Error **errp)
                        "Memory region %s uses guest_memfd, "
                        "which is not supported with CPR.",
                        memory_region_name(new_block->mr));
-            migrate_add_blocker_modes(&new_block->cpr_blocker, errp,
-                                      MIG_MODE_CPR_TRANSFER, -1);
+            migrate_add_blocker_modes(&new_block->cpr_blocker,
+                                      BIT(MIG_MODE_CPR_TRANSFER), errp);
         }
     }
 
@@ -4462,8 +4462,8 @@ void ram_block_add_cpr_blocker(RAMBlock *rb, Error **errp)
                "Memory region %s is not compatible with CPR. share=on is "
                "required for memory-backend objects, and aux-ram-share=on is "
                "required.", memory_region_name(rb->mr));
-    migrate_add_blocker_modes(&rb->cpr_blocker, errp, MIG_MODE_CPR_TRANSFER,
-                              -1);
+    migrate_add_blocker_modes(&rb->cpr_blocker, BIT(MIG_MODE_CPR_TRANSFER),
+                              errp);
 }
 
 void ram_block_del_cpr_blocker(RAMBlock *rb)
-- 
2.49.0

[PATCH 3/3] migration: Put Error **errp parameter last

[Markus Armbruster]

qapi/error.h's big comment:

 * - Functions that use Error to report errors have an Error **errp
 *   parameter.  It should be the last parameter, except for functions
 *   taking variable arguments.

is_only_migratable() and add_blockers() have it in the middle.  Clean
them up.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 migration/migration.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 5f1c83aafd..8a06b54167 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1867,7 +1867,7 @@ static bool is_busy(Error **reasonp, Error **errp)
     return false;
 }
 
-static bool is_only_migratable(Error **reasonp, Error **errp, unsigned modes)
+static bool is_only_migratable(Error **reasonp, unsigned modes, Error **errp)
 {
     ERRP_GUARD();
 
@@ -1881,7 +1881,7 @@ static bool is_only_migratable(Error **reasonp, Error **errp, unsigned modes)
     return false;
 }
 
-static int add_blockers(Error **reasonp, Error **errp, unsigned modes)
+static int add_blockers(Error **reasonp, unsigned modes, Error **errp)
 {
     for (MigMode mode = 0; mode < MIG_MODE__MAX; mode++) {
         if (modes & BIT(mode)) {
@@ -1904,12 +1904,12 @@ int migrate_add_blocker_normal(Error **reasonp, Error **errp)
 
 int migrate_add_blocker_modes(Error **reasonp, unsigned modes, Error **errp)
 {
-    if (is_only_migratable(reasonp, errp, modes)) {
+    if (is_only_migratable(reasonp, modes, errp)) {
         return -EACCES;
     } else if (is_busy(reasonp, errp)) {
         return -EBUSY;
     }
-    return add_blockers(reasonp, errp, modes);
+    return add_blockers(reasonp, modes, errp);
 }
 
 int migrate_add_blocker_internal(Error **reasonp, Error **errp)
@@ -1919,7 +1919,7 @@ int migrate_add_blocker_internal(Error **reasonp, Error **errp)
     if (is_busy(reasonp, errp)) {
         return -EBUSY;
     }
-    return add_blockers(reasonp, errp, modes);
+    return add_blockers(reasonp, modes, errp);
 }
 
 void migrate_del_blocker(Error **reasonp)
-- 
2.49.0

Re: [PATCH 0/3] migration: Cleanup around MigMode sets

[Fabiano Rosas]

Markus Armbruster <armbru@redhat.com> writes:

> Markus Armbruster (3):
>   migration: Use unsigned instead of int for bit set of MigMode
>   migration: Use bitset of MigMode instead of variable arguments
>   migration: Put Error **errp parameter last
>
>  include/migration/blocker.h |  9 ++----
>  include/migration/misc.h    | 10 +++----
>  hw/vfio/container-legacy.c  |  6 ++--
>  hw/vfio/cpr-iommufd.c       |  6 ++--
>  hw/vfio/cpr-legacy.c        |  8 +++---
>  hw/vfio/cpr.c               |  5 ++--
>  hw/vfio/device.c            |  4 +--
>  migration/migration.c       | 57 +++++++++----------------------------
>  stubs/migr-blocker.c        |  2 +-
>  system/physmem.c            |  8 +++---
>  10 files changed, 40 insertions(+), 75 deletions(-)

Series:

Reviewed-by: Fabiano Rosas <farosas@suse.de>

Re: [PATCH 0/3] migration: Cleanup around MigMode sets

[Peter Xu]

On Mon, Oct 27, 2025 at 10:24:47AM -0300, Fabiano Rosas wrote:
> Markus Armbruster <armbru@redhat.com> writes:
> 
> > Markus Armbruster (3):
> >   migration: Use unsigned instead of int for bit set of MigMode
> >   migration: Use bitset of MigMode instead of variable arguments
> >   migration: Put Error **errp parameter last
> >
> >  include/migration/blocker.h |  9 ++----
> >  include/migration/misc.h    | 10 +++----
> >  hw/vfio/container-legacy.c  |  6 ++--
> >  hw/vfio/cpr-iommufd.c       |  6 ++--
> >  hw/vfio/cpr-legacy.c        |  8 +++---
> >  hw/vfio/cpr.c               |  5 ++--
> >  hw/vfio/device.c            |  4 +--
> >  migration/migration.c       | 57 +++++++++----------------------------
> >  stubs/migr-blocker.c        |  2 +-
> >  system/physmem.c            |  8 +++---
> >  10 files changed, 40 insertions(+), 75 deletions(-)
> 
> Series:
> 
> Reviewed-by: Fabiano Rosas <farosas@suse.de>

I recall I tried to convince Steve to use a bitmask like what patch2 does
but failed.. I didn't push that too hard, though.

Queued, thanks both!

-- 
Peter Xu