* [PATCH v2 1/9] migration/options.c: Don't export migrate_tls_opts_free
2026-02-02 22:40 [PATCH v2 0/9] qapi: Use visitors for migration parameters handling Fabiano Rosas
@ 2026-02-02 22:40 ` Fabiano Rosas
2026-02-05 20:19 ` Peter Xu
2026-02-02 22:40 ` [PATCH v2 2/9] migration: Use QAPI_CLONE_MEMBERS in migrate_params_test_apply Fabiano Rosas
` (8 subsequent siblings)
9 siblings, 1 reply; 28+ messages in thread
From: Fabiano Rosas @ 2026-02-02 22:40 UTC (permalink / raw)
To: qemu-devel; +Cc: peterx, armbru, ppandit
The migrate_tls_opts_free function was never used outside
options.c. Make it static.
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
migration/options.c | 2 +-
migration/options.h | 1 -
2 files changed, 1 insertion(+), 2 deletions(-)
diff --git a/migration/options.c b/migration/options.c
index 1ffe85a2d8..dcd5e07504 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -1001,7 +1001,7 @@ AnnounceParameters *migrate_announce_params(void)
return ≈
}
-void migrate_tls_opts_free(MigrationParameters *params)
+static void migrate_tls_opts_free(MigrationParameters *params)
{
qapi_free_StrOrNull(params->tls_creds);
qapi_free_StrOrNull(params->tls_hostname);
diff --git a/migration/options.h b/migration/options.h
index b502871097..0c3043f1ff 100644
--- a/migration/options.h
+++ b/migration/options.h
@@ -92,5 +92,4 @@ ZeroPageDetection migrate_zero_page_detection(void);
bool migrate_params_check(MigrationParameters *params, Error **errp);
void migrate_params_init(MigrationParameters *params);
-void migrate_tls_opts_free(MigrationParameters *params);
#endif
--
2.51.0
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH v2 2/9] migration: Use QAPI_CLONE_MEMBERS in migrate_params_test_apply
2026-02-02 22:40 [PATCH v2 0/9] qapi: Use visitors for migration parameters handling Fabiano Rosas
2026-02-02 22:40 ` [PATCH v2 1/9] migration/options.c: Don't export migrate_tls_opts_free Fabiano Rosas
@ 2026-02-02 22:40 ` Fabiano Rosas
2026-02-05 20:19 ` Peter Xu
2026-02-02 22:40 ` [PATCH v2 3/9] migration: Use QAPI_CLONE_MEMBERS in migrate_params_apply Fabiano Rosas
` (7 subsequent siblings)
9 siblings, 1 reply; 28+ messages in thread
From: Fabiano Rosas @ 2026-02-02 22:40 UTC (permalink / raw)
To: qemu-devel; +Cc: peterx, armbru, ppandit
Use QAPI_CLONE_MEMBERS instead of making an assignment. The QAPI
method makes the handling of the TLS strings more intuitive because it
clones them as well.
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
migration/options.c | 26 ++++++++++++--------------
1 file changed, 12 insertions(+), 14 deletions(-)
diff --git a/migration/options.c b/migration/options.c
index dcd5e07504..4161648268 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -1264,9 +1264,9 @@ bool migrate_params_check(MigrationParameters *params, Error **errp)
static void migrate_params_test_apply(MigrationParameters *params,
MigrationParameters *dest)
{
- *dest = migrate_get_current()->parameters;
+ MigrationState *s = migrate_get_current();
- /* TODO use QAPI_CLONE() instead of duplicating it inline */
+ QAPI_CLONE_MEMBERS(MigrationParameters, dest, &s->parameters);
if (params->has_throttle_trigger_threshold) {
dest->throttle_trigger_threshold = params->throttle_trigger_threshold;
@@ -1285,24 +1285,18 @@ static void migrate_params_test_apply(MigrationParameters *params,
}
if (params->tls_creds) {
+ qapi_free_StrOrNull(dest->tls_creds);
dest->tls_creds = QAPI_CLONE(StrOrNull, params->tls_creds);
- } else {
- /* clear the reference, it's owned by s->parameters */
- dest->tls_creds = NULL;
}
if (params->tls_hostname) {
+ qapi_free_StrOrNull(dest->tls_hostname);
dest->tls_hostname = QAPI_CLONE(StrOrNull, params->tls_hostname);
- } else {
- /* clear the reference, it's owned by s->parameters */
- dest->tls_hostname = NULL;
}
if (params->tls_authz) {
+ qapi_free_StrOrNull(dest->tls_authz);
dest->tls_authz = QAPI_CLONE(StrOrNull, params->tls_authz);
- } else {
- /* clear the reference, it's owned by s->parameters */
- dest->tls_authz = NULL;
}
if (params->has_max_bandwidth) {
@@ -1359,8 +1353,9 @@ static void migrate_params_test_apply(MigrationParameters *params,
}
if (params->has_block_bitmap_mapping) {
- dest->has_block_bitmap_mapping = true;
- dest->block_bitmap_mapping = params->block_bitmap_mapping;
+ qapi_free_BitmapMigrationNodeAliasList(dest->block_bitmap_mapping);
+ dest->block_bitmap_mapping = QAPI_CLONE(BitmapMigrationNodeAliasList,
+ params->block_bitmap_mapping);
}
if (params->has_x_vcpu_dirty_limit_period) {
@@ -1384,7 +1379,8 @@ static void migrate_params_test_apply(MigrationParameters *params,
}
if (params->has_cpr_exec_command) {
- dest->cpr_exec_command = params->cpr_exec_command;
+ qapi_free_strList(dest->cpr_exec_command);
+ dest->cpr_exec_command = QAPI_CLONE(strList, params->cpr_exec_command);
}
}
@@ -1540,4 +1536,6 @@ void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
}
migrate_tls_opts_free(&tmp);
+ qapi_free_BitmapMigrationNodeAliasList(tmp.block_bitmap_mapping);
+ qapi_free_strList(tmp.cpr_exec_command);
}
--
2.51.0
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH v2 2/9] migration: Use QAPI_CLONE_MEMBERS in migrate_params_test_apply
2026-02-02 22:40 ` [PATCH v2 2/9] migration: Use QAPI_CLONE_MEMBERS in migrate_params_test_apply Fabiano Rosas
@ 2026-02-05 20:19 ` Peter Xu
0 siblings, 0 replies; 28+ messages in thread
From: Peter Xu @ 2026-02-05 20:19 UTC (permalink / raw)
To: Fabiano Rosas; +Cc: qemu-devel, armbru, ppandit
On Mon, Feb 02, 2026 at 07:40:54PM -0300, Fabiano Rosas wrote:
> Use QAPI_CLONE_MEMBERS instead of making an assignment. The QAPI
> method makes the handling of the TLS strings more intuitive because it
> clones them as well.
>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
> migration/options.c | 26 ++++++++++++--------------
> 1 file changed, 12 insertions(+), 14 deletions(-)
>
> diff --git a/migration/options.c b/migration/options.c
> index dcd5e07504..4161648268 100644
> --- a/migration/options.c
> +++ b/migration/options.c
> @@ -1264,9 +1264,9 @@ bool migrate_params_check(MigrationParameters *params, Error **errp)
> static void migrate_params_test_apply(MigrationParameters *params,
> MigrationParameters *dest)
> {
> - *dest = migrate_get_current()->parameters;
> + MigrationState *s = migrate_get_current();
>
> - /* TODO use QAPI_CLONE() instead of duplicating it inline */
> + QAPI_CLONE_MEMBERS(MigrationParameters, dest, &s->parameters);
>
> if (params->has_throttle_trigger_threshold) {
> dest->throttle_trigger_threshold = params->throttle_trigger_threshold;
> @@ -1285,24 +1285,18 @@ static void migrate_params_test_apply(MigrationParameters *params,
> }
>
> if (params->tls_creds) {
> + qapi_free_StrOrNull(dest->tls_creds);
> dest->tls_creds = QAPI_CLONE(StrOrNull, params->tls_creds);
> - } else {
> - /* clear the reference, it's owned by s->parameters */
> - dest->tls_creds = NULL;
> }
>
> if (params->tls_hostname) {
> + qapi_free_StrOrNull(dest->tls_hostname);
> dest->tls_hostname = QAPI_CLONE(StrOrNull, params->tls_hostname);
> - } else {
> - /* clear the reference, it's owned by s->parameters */
> - dest->tls_hostname = NULL;
> }
>
> if (params->tls_authz) {
> + qapi_free_StrOrNull(dest->tls_authz);
> dest->tls_authz = QAPI_CLONE(StrOrNull, params->tls_authz);
> - } else {
> - /* clear the reference, it's owned by s->parameters */
> - dest->tls_authz = NULL;
> }
>
> if (params->has_max_bandwidth) {
> @@ -1359,8 +1353,9 @@ static void migrate_params_test_apply(MigrationParameters *params,
> }
>
> if (params->has_block_bitmap_mapping) {
> - dest->has_block_bitmap_mapping = true;
Changing this line seems all fine, IIUC it's because migrate_params_check()
always checks block_bitmap_mapping directly ignoring the has_* here,
meanwhile if check passed, migrate_params_apply() will read the has_* but
only read the one in params.
I wonder if we want to split a separate patch removing this line only, or
maybe add a comment into commit log for this change (can be done when merge
too if this is the only change).
The change itself looks good:
Reviewed-by: Peter Xu <peterx@redhat.com>
> - dest->block_bitmap_mapping = params->block_bitmap_mapping;
> + qapi_free_BitmapMigrationNodeAliasList(dest->block_bitmap_mapping);
> + dest->block_bitmap_mapping = QAPI_CLONE(BitmapMigrationNodeAliasList,
> + params->block_bitmap_mapping);
> }
>
> if (params->has_x_vcpu_dirty_limit_period) {
> @@ -1384,7 +1379,8 @@ static void migrate_params_test_apply(MigrationParameters *params,
> }
>
> if (params->has_cpr_exec_command) {
> - dest->cpr_exec_command = params->cpr_exec_command;
> + qapi_free_strList(dest->cpr_exec_command);
> + dest->cpr_exec_command = QAPI_CLONE(strList, params->cpr_exec_command);
> }
> }
>
> @@ -1540,4 +1536,6 @@ void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
> }
>
> migrate_tls_opts_free(&tmp);
> + qapi_free_BitmapMigrationNodeAliasList(tmp.block_bitmap_mapping);
> + qapi_free_strList(tmp.cpr_exec_command);
> }
> --
> 2.51.0
>
--
Peter Xu
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 3/9] migration: Use QAPI_CLONE_MEMBERS in migrate_params_apply
2026-02-02 22:40 [PATCH v2 0/9] qapi: Use visitors for migration parameters handling Fabiano Rosas
2026-02-02 22:40 ` [PATCH v2 1/9] migration/options.c: Don't export migrate_tls_opts_free Fabiano Rosas
2026-02-02 22:40 ` [PATCH v2 2/9] migration: Use QAPI_CLONE_MEMBERS in migrate_params_test_apply Fabiano Rosas
@ 2026-02-02 22:40 ` Fabiano Rosas
2026-02-02 22:40 ` [PATCH v2 4/9] qapi: Implement qapi_dealloc_present_visitor Fabiano Rosas
` (6 subsequent siblings)
9 siblings, 0 replies; 28+ messages in thread
From: Fabiano Rosas @ 2026-02-02 22:40 UTC (permalink / raw)
To: qemu-devel; +Cc: peterx, armbru, ppandit, Prasad Pandit
Instead of setting parameters one by one, use the temporary object,
which already contains the current migration parameters plus the new
ones and was just validated by migration_params_check(). Use cloning
to overwrite it.
This avoids the need to alter this function every time a new parameter
is added.
Since parameters are not individually checked anymore, the setting of
s->has_block_bitmap_mapping moves into migrate_post_update_params().
Reviewed-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Prasad Pandit <pjp@fedoraproject.org>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
migration/options.c | 137 +++++---------------------------------------
1 file changed, 14 insertions(+), 123 deletions(-)
diff --git a/migration/options.c b/migration/options.c
index 4161648268..3c80cdfd3a 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -1106,6 +1106,10 @@ static void migrate_post_update_params(MigrationParameters *new, Error **errp)
migration_rate_set(new->max_postcopy_bandwidth);
}
}
+
+ if (new->has_block_bitmap_mapping) {
+ s->has_block_bitmap_mapping = true;
+ }
}
/*
@@ -1384,132 +1388,19 @@ static void migrate_params_test_apply(MigrationParameters *params,
}
}
+/*
+ * Caller must ensure the has_* fields of @params are true so they all
+ * get copied and the pointer members don't dangle.
+ */
static void migrate_params_apply(MigrationParameters *params)
{
MigrationState *s = migrate_get_current();
+ MigrationParameters *cur = &s->parameters;
- /* TODO use QAPI_CLONE() instead of duplicating it inline */
-
- if (params->has_throttle_trigger_threshold) {
- s->parameters.throttle_trigger_threshold = params->throttle_trigger_threshold;
- }
-
- if (params->has_cpu_throttle_initial) {
- s->parameters.cpu_throttle_initial = params->cpu_throttle_initial;
- }
-
- if (params->has_cpu_throttle_increment) {
- s->parameters.cpu_throttle_increment = params->cpu_throttle_increment;
- }
-
- if (params->has_cpu_throttle_tailslow) {
- s->parameters.cpu_throttle_tailslow = params->cpu_throttle_tailslow;
- }
-
- if (params->tls_creds) {
- qapi_free_StrOrNull(s->parameters.tls_creds);
- s->parameters.tls_creds = QAPI_CLONE(StrOrNull, params->tls_creds);
- }
-
- if (params->tls_hostname) {
- qapi_free_StrOrNull(s->parameters.tls_hostname);
- s->parameters.tls_hostname = QAPI_CLONE(StrOrNull,
- params->tls_hostname);
- }
-
- if (params->tls_authz) {
- qapi_free_StrOrNull(s->parameters.tls_authz);
- s->parameters.tls_authz = QAPI_CLONE(StrOrNull, params->tls_authz);
- }
-
- if (params->has_max_bandwidth) {
- s->parameters.max_bandwidth = params->max_bandwidth;
- }
-
- if (params->has_avail_switchover_bandwidth) {
- s->parameters.avail_switchover_bandwidth = params->avail_switchover_bandwidth;
- }
-
- if (params->has_downtime_limit) {
- s->parameters.downtime_limit = params->downtime_limit;
- }
-
- if (params->has_x_checkpoint_delay) {
- s->parameters.x_checkpoint_delay = params->x_checkpoint_delay;
- }
-
- if (params->has_multifd_channels) {
- s->parameters.multifd_channels = params->multifd_channels;
- }
- if (params->has_multifd_compression) {
- s->parameters.multifd_compression = params->multifd_compression;
- }
- if (params->has_multifd_qatzip_level) {
- s->parameters.multifd_qatzip_level = params->multifd_qatzip_level;
- }
- if (params->has_multifd_zlib_level) {
- s->parameters.multifd_zlib_level = params->multifd_zlib_level;
- }
- if (params->has_multifd_zstd_level) {
- s->parameters.multifd_zstd_level = params->multifd_zstd_level;
- }
- if (params->has_xbzrle_cache_size) {
- s->parameters.xbzrle_cache_size = params->xbzrle_cache_size;
- }
- if (params->has_max_postcopy_bandwidth) {
- s->parameters.max_postcopy_bandwidth = params->max_postcopy_bandwidth;
- }
- if (params->has_max_cpu_throttle) {
- s->parameters.max_cpu_throttle = params->max_cpu_throttle;
- }
- if (params->has_announce_initial) {
- s->parameters.announce_initial = params->announce_initial;
- }
- if (params->has_announce_max) {
- s->parameters.announce_max = params->announce_max;
- }
- if (params->has_announce_rounds) {
- s->parameters.announce_rounds = params->announce_rounds;
- }
- if (params->has_announce_step) {
- s->parameters.announce_step = params->announce_step;
- }
-
- if (params->has_block_bitmap_mapping) {
- qapi_free_BitmapMigrationNodeAliasList(
- s->parameters.block_bitmap_mapping);
-
- s->has_block_bitmap_mapping = true;
- s->parameters.block_bitmap_mapping =
- QAPI_CLONE(BitmapMigrationNodeAliasList,
- params->block_bitmap_mapping);
- }
-
- if (params->has_x_vcpu_dirty_limit_period) {
- s->parameters.x_vcpu_dirty_limit_period =
- params->x_vcpu_dirty_limit_period;
- }
- if (params->has_vcpu_dirty_limit) {
- s->parameters.vcpu_dirty_limit = params->vcpu_dirty_limit;
- }
-
- if (params->has_mode) {
- s->parameters.mode = params->mode;
- }
-
- if (params->has_zero_page_detection) {
- s->parameters.zero_page_detection = params->zero_page_detection;
- }
-
- if (params->has_direct_io) {
- s->parameters.direct_io = params->direct_io;
- }
-
- if (params->has_cpr_exec_command) {
- qapi_free_strList(s->parameters.cpr_exec_command);
- s->parameters.cpr_exec_command =
- QAPI_CLONE(strList, params->cpr_exec_command);
- }
+ migrate_tls_opts_free(cur);
+ qapi_free_BitmapMigrationNodeAliasList(cur->block_bitmap_mapping);
+ qapi_free_strList(cur->cpr_exec_command);
+ QAPI_CLONE_MEMBERS(MigrationParameters, cur, params);
}
void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
@@ -1531,7 +1422,7 @@ void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
migrate_params_test_apply(params, &tmp);
if (migrate_params_check(&tmp, errp)) {
- migrate_params_apply(params);
+ migrate_params_apply(&tmp);
migrate_post_update_params(params, errp);
}
--
2.51.0
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH v2 4/9] qapi: Implement qapi_dealloc_present_visitor
2026-02-02 22:40 [PATCH v2 0/9] qapi: Use visitors for migration parameters handling Fabiano Rosas
` (2 preceding siblings ...)
2026-02-02 22:40 ` [PATCH v2 3/9] migration: Use QAPI_CLONE_MEMBERS in migrate_params_apply Fabiano Rosas
@ 2026-02-02 22:40 ` Fabiano Rosas
2026-02-05 21:45 ` Peter Xu
2026-02-02 22:40 ` [PATCH v2 5/9] qapi: Add QAPI_MERGE Fabiano Rosas
` (5 subsequent siblings)
9 siblings, 1 reply; 28+ messages in thread
From: Fabiano Rosas @ 2026-02-02 22:40 UTC (permalink / raw)
To: qemu-devel; +Cc: peterx, armbru, ppandit, Michael Roth
Implement a visitor that frees the pointer members of the visited QAPI
object in the same way that qapi_dealloc_visitor does, but similarly
to qobject_input_visitor, takes an input QObject that will dictate
which members get freed and which don't. Members not present in the
input QObject will be left unchanged in the visited QAPI object.
This is useful to free memory just before perfoming a visit with
qobject_input_visitor on a pre-existing, non-null QAPI object. If the
same QObject is passed to both visitors, the pointers overwritten by
the input visitor match the ones that are freed by the dealloc
visitor.
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
include/qapi/dealloc-visitor.h | 6 ++
qapi/qapi-dealloc-visitor.c | 173 ++++++++++++++++++++++++++++++++-
2 files changed, 178 insertions(+), 1 deletion(-)
diff --git a/include/qapi/dealloc-visitor.h b/include/qapi/dealloc-visitor.h
index c36715fdf3..96c7bf35c3 100644
--- a/include/qapi/dealloc-visitor.h
+++ b/include/qapi/dealloc-visitor.h
@@ -25,4 +25,10 @@ typedef struct QapiDeallocVisitor QapiDeallocVisitor;
*/
Visitor *qapi_dealloc_visitor_new(void);
+/*
+ * Like qapi_dealloc_visitor_new but visits a QObject and only frees
+ * present members.
+ */
+Visitor *qapi_dealloc_present_visitor_new(QObject *);
+
#endif
diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
index 57a2c904bb..90b017cc93 100644
--- a/qapi/qapi-dealloc-visitor.c
+++ b/qapi/qapi-dealloc-visitor.c
@@ -14,14 +14,146 @@
#include "qemu/osdep.h"
#include "qapi/dealloc-visitor.h"
+#include "qemu/queue.h"
+#include "qobject/qdict.h"
+#include "qobject/qlist.h"
#include "qobject/qnull.h"
#include "qapi/visitor-impl.h"
+typedef struct QStackEntry {
+ QObject *obj; /* QDict or QList being visited */
+ void *qapi;
+ const QListEntry *entry; /* If @obj is QList: unvisited tail */
+ QSLIST_ENTRY(QStackEntry) node;
+} QStackEntry;
+
struct QapiDeallocVisitor
{
Visitor visitor;
+ QObject *root;
+ QSLIST_HEAD(, QStackEntry) stack;
};
+static void qapi_dealloc_pop(Visitor *v, void **obj)
+{
+ QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
+ QStackEntry *se = QSLIST_FIRST(&qdv->stack);
+
+ assert(se && se->qapi == obj);
+ QSLIST_REMOVE_HEAD(&qdv->stack, node);
+ g_free(se);
+}
+
+static void qapi_dealloc_push(Visitor *v, QObject *obj, void *qapi)
+{
+ QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
+ QStackEntry *se = g_new0(QStackEntry, 1);
+
+ assert(obj);
+ se->obj = obj;
+ se->qapi = qapi;
+
+ if (qobject_type(obj) == QTYPE_QLIST) {
+ se->entry = qlist_first(qobject_to(QList, obj));
+ }
+
+ QSLIST_INSERT_HEAD(&qdv->stack, se, node);
+}
+
+static QObject *qapi_dealloc_try_get_object(QapiDeallocVisitor *qdv, const char *name)
+{
+ QStackEntry *se = QSLIST_FIRST(&qdv->stack);
+ QObject *qobj;
+ QObject *ret = NULL;
+
+ if (!se) {
+ assert(qdv->root);
+ return qdv->root;
+ }
+
+ qobj = se->obj;
+ assert(qobj);
+
+ if (qobject_type(qobj) == QTYPE_QDICT) {
+ assert(name);
+ ret = qdict_get(qobject_to(QDict, qobj), name);
+ } else {
+ assert(qobject_type(qobj) == QTYPE_QLIST);
+ assert(!name);
+ if (se->entry) {
+ ret = qlist_entry_obj(se->entry);
+ }
+ }
+
+ return ret;
+}
+
+static bool qapi_dealloc_present_start_struct(Visitor *v, const char *name,
+ void **obj, size_t size,
+ Error **errp)
+{
+ QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
+ QObject *qobj = qapi_dealloc_try_get_object(qdv, name);
+
+ if (!qobj) {
+ return false;
+ }
+ assert(qobject_type(qobj) == QTYPE_QDICT);
+ qapi_dealloc_push(v, qobj, obj);
+ return true;
+}
+
+static void qapi_dealloc_present_end_struct(Visitor *v, void **obj)
+{
+ QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
+ QStackEntry *se = QSLIST_FIRST(&qdv->stack);
+
+ assert(qobject_type(se->obj) == QTYPE_QDICT);
+ qapi_dealloc_pop(v, obj);
+
+ if (obj) {
+ g_free(*obj);
+ }
+}
+
+static bool qapi_dealloc_present_start_list(Visitor *v, const char *name,
+ GenericList **list, size_t size,
+ Error **errp)
+{
+ QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
+ QObject *qobj = qapi_dealloc_try_get_object(qdv, name);
+
+ if (!qobj) {
+ return false;
+ }
+ assert(qobject_type(qobj) == QTYPE_QLIST);
+ qapi_dealloc_push(v, qobj, list);
+ return true;
+}
+
+static void qapi_dealloc_present_end_list(Visitor *v, void **obj)
+{
+ QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
+ QStackEntry *se = QSLIST_FIRST(&qdv->stack);
+
+ assert(qobject_type(se->obj) == QTYPE_QLIST);
+ qapi_dealloc_pop(v, obj);
+}
+
+static void qapi_dealloc_present_free(Visitor *v)
+{
+ QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
+
+ while (!QSLIST_EMPTY(&qdv->stack)) {
+ QStackEntry *se = QSLIST_FIRST(&qdv->stack);
+
+ QSLIST_REMOVE_HEAD(&qdv->stack, node);
+ g_free(se);
+ }
+ qobject_unref(qdv->root);
+ g_free(qdv);
+}
+
static bool qapi_dealloc_start_struct(Visitor *v, const char *name, void **obj,
size_t unused, Error **errp)
{
@@ -35,6 +167,21 @@ static void qapi_dealloc_end_struct(Visitor *v, void **obj)
}
}
+static bool qapi_dealloc_start_alternate(Visitor *v, const char *name,
+ GenericAlternate **obj, size_t size,
+ Error **errp)
+{
+ QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
+ QObject *qobj = qapi_dealloc_try_get_object(qdv, name);
+
+ if (!qobj) {
+ return false;
+ }
+ assert(*obj);
+ (*obj)->type = qobject_type(qobj);
+ return true;
+}
+
static void qapi_dealloc_end_alternate(Visitor *v, void **obj)
{
if (obj) {
@@ -117,13 +264,14 @@ static void qapi_dealloc_free(Visitor *v)
g_free(container_of(v, QapiDeallocVisitor, visitor));
}
-Visitor *qapi_dealloc_visitor_new(void)
+static QapiDeallocVisitor *qapi_dealloc_visitor_new_base(void)
{
QapiDeallocVisitor *v;
v = g_malloc0(sizeof(*v));
v->visitor.type = VISITOR_DEALLOC;
+
v->visitor.start_struct = qapi_dealloc_start_struct;
v->visitor.end_struct = qapi_dealloc_end_struct;
v->visitor.end_alternate = qapi_dealloc_end_alternate;
@@ -139,5 +287,28 @@ Visitor *qapi_dealloc_visitor_new(void)
v->visitor.type_null = qapi_dealloc_type_null;
v->visitor.free = qapi_dealloc_free;
+ return v;
+}
+
+Visitor *qapi_dealloc_visitor_new(void)
+{
+ QapiDeallocVisitor *v = qapi_dealloc_visitor_new_base();
+
+ return &v->visitor;
+}
+
+Visitor *qapi_dealloc_present_visitor_new(QObject *obj)
+{
+ QapiDeallocVisitor *v = qapi_dealloc_visitor_new_base();
+
+ v->visitor.start_alternate = qapi_dealloc_start_alternate;
+ v->visitor.start_list = qapi_dealloc_present_start_list;
+ v->visitor.end_list = qapi_dealloc_present_end_list;
+ v->visitor.start_struct = qapi_dealloc_present_start_struct;
+ v->visitor.end_struct = qapi_dealloc_present_end_struct;
+ v->visitor.free = qapi_dealloc_present_free;
+
+ v->root = qobject_ref(obj);
+
return &v->visitor;
}
--
2.51.0
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH v2 4/9] qapi: Implement qapi_dealloc_present_visitor
2026-02-02 22:40 ` [PATCH v2 4/9] qapi: Implement qapi_dealloc_present_visitor Fabiano Rosas
@ 2026-02-05 21:45 ` Peter Xu
2026-02-06 12:19 ` Fabiano Rosas
0 siblings, 1 reply; 28+ messages in thread
From: Peter Xu @ 2026-02-05 21:45 UTC (permalink / raw)
To: Fabiano Rosas; +Cc: qemu-devel, armbru, ppandit, Michael Roth
On Mon, Feb 02, 2026 at 07:40:56PM -0300, Fabiano Rosas wrote:
> Implement a visitor that frees the pointer members of the visited QAPI
> object in the same way that qapi_dealloc_visitor does, but similarly
> to qobject_input_visitor, takes an input QObject that will dictate
> which members get freed and which don't. Members not present in the
> input QObject will be left unchanged in the visited QAPI object.
>
> This is useful to free memory just before perfoming a visit with
> qobject_input_visitor on a pre-existing, non-null QAPI object. If the
> same QObject is passed to both visitors, the pointers overwritten by
> the input visitor match the ones that are freed by the dealloc
> visitor.
>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
> include/qapi/dealloc-visitor.h | 6 ++
> qapi/qapi-dealloc-visitor.c | 173 ++++++++++++++++++++++++++++++++-
First of all, I saw that all prior visitors should always have unit tests
under tests/unit/. Maybe we should also attach an unit test?
> 2 files changed, 178 insertions(+), 1 deletion(-)
>
> diff --git a/include/qapi/dealloc-visitor.h b/include/qapi/dealloc-visitor.h
> index c36715fdf3..96c7bf35c3 100644
> --- a/include/qapi/dealloc-visitor.h
> +++ b/include/qapi/dealloc-visitor.h
> @@ -25,4 +25,10 @@ typedef struct QapiDeallocVisitor QapiDeallocVisitor;
> */
> Visitor *qapi_dealloc_visitor_new(void);
>
> +/*
> + * Like qapi_dealloc_visitor_new but visits a QObject and only frees
> + * present members.
> + */
> +Visitor *qapi_dealloc_present_visitor_new(QObject *);
> +
> #endif
> diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
> index 57a2c904bb..90b017cc93 100644
> --- a/qapi/qapi-dealloc-visitor.c
> +++ b/qapi/qapi-dealloc-visitor.c
> @@ -14,14 +14,146 @@
>
> #include "qemu/osdep.h"
> #include "qapi/dealloc-visitor.h"
> +#include "qemu/queue.h"
> +#include "qobject/qdict.h"
> +#include "qobject/qlist.h"
> #include "qobject/qnull.h"
> #include "qapi/visitor-impl.h"
>
> +typedef struct QStackEntry {
> + QObject *obj; /* QDict or QList being visited */
> + void *qapi;
This one is only for debugging purpose, but not required, right?
I wonder if we could just rely on the unit test for correctness, otherwise
we kind of run some testing code with/without --enable-debug. Not a huge
deal I think.. so see this a pure question.
> + const QListEntry *entry; /* If @obj is QList: unvisited tail */
> + QSLIST_ENTRY(QStackEntry) node;
> +} QStackEntry;
> +
> struct QapiDeallocVisitor
> {
> Visitor visitor;
> + QObject *root;
> + QSLIST_HEAD(, QStackEntry) stack;
> };
>
> +static void qapi_dealloc_pop(Visitor *v, void **obj)
> +{
> + QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
> + QStackEntry *se = QSLIST_FIRST(&qdv->stack);
> +
> + assert(se && se->qapi == obj);
> + QSLIST_REMOVE_HEAD(&qdv->stack, node);
> + g_free(se);
> +}
> +
> +static void qapi_dealloc_push(Visitor *v, QObject *obj, void *qapi)
> +{
> + QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
> + QStackEntry *se = g_new0(QStackEntry, 1);
> +
> + assert(obj);
> + se->obj = obj;
> + se->qapi = qapi;
> +
> + if (qobject_type(obj) == QTYPE_QLIST) {
> + se->entry = qlist_first(qobject_to(QList, obj));
I still don't yet understand why do we care about lists here.
I'm trying to guess, what this code wanted to do: we pushed the 1st entry
of the list into the stack, trying to make it as a reference for all the
items later within the list.
But IIUC we can still define the conditiona-dealloc visitor to be even
simpler, right? Say, if the ref qobject has the qlist object, then free
the whole list?
IMHO there're three things we need to manage on conditional deallocations:
(1) struct, (2) list, (3) alternatives. All the rest seem to be scalars.
So I wonder if we could define this visitor, so that it treats both (2) and
(3) the same way (to always dealloc as long as present).
That sounds at least making more sense when I picture that in migration
parameters: if we have a list in the parameters (e.g. cpr-exec-command), as
long as the list is present in the ref, we should free the whole thing
completely on the other one being visited.
> + }
> +
> + QSLIST_INSERT_HEAD(&qdv->stack, se, node);
> +}
> +
> +static QObject *qapi_dealloc_try_get_object(QapiDeallocVisitor *qdv, const char *name)
> +{
> + QStackEntry *se = QSLIST_FIRST(&qdv->stack);
> + QObject *qobj;
> + QObject *ret = NULL;
> +
> + if (!se) {
> + assert(qdv->root);
> + return qdv->root;
> + }
> +
> + qobj = se->obj;
> + assert(qobj);
> +
> + if (qobject_type(qobj) == QTYPE_QDICT) {
> + assert(name);
> + ret = qdict_get(qobject_to(QDict, qobj), name);
> + } else {
> + assert(qobject_type(qobj) == QTYPE_QLIST);
> + assert(!name);
> + if (se->entry) {
> + ret = qlist_entry_obj(se->entry);
> + }
> + }
> +
> + return ret;
> +}
> +
> +static bool qapi_dealloc_present_start_struct(Visitor *v, const char *name,
> + void **obj, size_t size,
> + Error **errp)
> +{
> + QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
> + QObject *qobj = qapi_dealloc_try_get_object(qdv, name);
> +
> + if (!qobj) {
> + return false;
> + }
> + assert(qobject_type(qobj) == QTYPE_QDICT);
> + qapi_dealloc_push(v, qobj, obj);
> + return true;
> +}
> +
> +static void qapi_dealloc_present_end_struct(Visitor *v, void **obj)
> +{
> + QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
> + QStackEntry *se = QSLIST_FIRST(&qdv->stack);
> +
> + assert(qobject_type(se->obj) == QTYPE_QDICT);
> + qapi_dealloc_pop(v, obj);
> +
> + if (obj) {
> + g_free(*obj);
> + }
> +}
> +
> +static bool qapi_dealloc_present_start_list(Visitor *v, const char *name,
> + GenericList **list, size_t size,
> + Error **errp)
> +{
> + QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
> + QObject *qobj = qapi_dealloc_try_get_object(qdv, name);
> +
> + if (!qobj) {
> + return false;
> + }
> + assert(qobject_type(qobj) == QTYPE_QLIST);
> + qapi_dealloc_push(v, qobj, list);
> + return true;
> +}
> +
> +static void qapi_dealloc_present_end_list(Visitor *v, void **obj)
> +{
> + QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
> + QStackEntry *se = QSLIST_FIRST(&qdv->stack);
> +
> + assert(qobject_type(se->obj) == QTYPE_QLIST);
> + qapi_dealloc_pop(v, obj);
> +}
> +
> +static void qapi_dealloc_present_free(Visitor *v)
> +{
> + QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
> +
> + while (!QSLIST_EMPTY(&qdv->stack)) {
> + QStackEntry *se = QSLIST_FIRST(&qdv->stack);
> +
> + QSLIST_REMOVE_HEAD(&qdv->stack, node);
> + g_free(se);
> + }
> + qobject_unref(qdv->root);
> + g_free(qdv);
> +}
> +
> static bool qapi_dealloc_start_struct(Visitor *v, const char *name, void **obj,
> size_t unused, Error **errp)
> {
> @@ -35,6 +167,21 @@ static void qapi_dealloc_end_struct(Visitor *v, void **obj)
> }
> }
>
> +static bool qapi_dealloc_start_alternate(Visitor *v, const char *name,
> + GenericAlternate **obj, size_t size,
> + Error **errp)
> +{
> + QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
> + QObject *qobj = qapi_dealloc_try_get_object(qdv, name);
> +
> + if (!qobj) {
> + return false;
> + }
> + assert(*obj);
> + (*obj)->type = qobject_type(qobj);
> + return true;
> +}
> +
> static void qapi_dealloc_end_alternate(Visitor *v, void **obj)
> {
> if (obj) {
> @@ -117,13 +264,14 @@ static void qapi_dealloc_free(Visitor *v)
> g_free(container_of(v, QapiDeallocVisitor, visitor));
> }
>
> -Visitor *qapi_dealloc_visitor_new(void)
> +static QapiDeallocVisitor *qapi_dealloc_visitor_new_base(void)
> {
> QapiDeallocVisitor *v;
>
> v = g_malloc0(sizeof(*v));
>
> v->visitor.type = VISITOR_DEALLOC;
> +
> v->visitor.start_struct = qapi_dealloc_start_struct;
> v->visitor.end_struct = qapi_dealloc_end_struct;
> v->visitor.end_alternate = qapi_dealloc_end_alternate;
> @@ -139,5 +287,28 @@ Visitor *qapi_dealloc_visitor_new(void)
> v->visitor.type_null = qapi_dealloc_type_null;
> v->visitor.free = qapi_dealloc_free;
IMHO, setting the hooks once then overwrite, is less clean than moving them
into qapi_dealloc_visitor_new() if dealloc_present visitor will do that.
>
> + return v;
> +}
> +
> +Visitor *qapi_dealloc_visitor_new(void)
> +{
> + QapiDeallocVisitor *v = qapi_dealloc_visitor_new_base();
> +
> + return &v->visitor;
> +}
> +
> +Visitor *qapi_dealloc_present_visitor_new(QObject *obj)
> +{
> + QapiDeallocVisitor *v = qapi_dealloc_visitor_new_base();
> +
> + v->visitor.start_alternate = qapi_dealloc_start_alternate;
> + v->visitor.start_list = qapi_dealloc_present_start_list;
> + v->visitor.end_list = qapi_dealloc_present_end_list;
> + v->visitor.start_struct = qapi_dealloc_present_start_struct;
> + v->visitor.end_struct = qapi_dealloc_present_end_struct;
> + v->visitor.free = qapi_dealloc_present_free;
> +
> + v->root = qobject_ref(obj);
> +
> return &v->visitor;
> }
> --
> 2.51.0
>
--
Peter Xu
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v2 4/9] qapi: Implement qapi_dealloc_present_visitor
2026-02-05 21:45 ` Peter Xu
@ 2026-02-06 12:19 ` Fabiano Rosas
2026-02-06 17:38 ` Peter Xu
0 siblings, 1 reply; 28+ messages in thread
From: Fabiano Rosas @ 2026-02-06 12:19 UTC (permalink / raw)
To: Peter Xu; +Cc: qemu-devel, armbru, ppandit, Michael Roth
Peter Xu <peterx@redhat.com> writes:
> On Mon, Feb 02, 2026 at 07:40:56PM -0300, Fabiano Rosas wrote:
>> Implement a visitor that frees the pointer members of the visited QAPI
>> object in the same way that qapi_dealloc_visitor does, but similarly
>> to qobject_input_visitor, takes an input QObject that will dictate
>> which members get freed and which don't. Members not present in the
>> input QObject will be left unchanged in the visited QAPI object.
>>
>> This is useful to free memory just before perfoming a visit with
>> qobject_input_visitor on a pre-existing, non-null QAPI object. If the
>> same QObject is passed to both visitors, the pointers overwritten by
>> the input visitor match the ones that are freed by the dealloc
>> visitor.
>>
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>> include/qapi/dealloc-visitor.h | 6 ++
>> qapi/qapi-dealloc-visitor.c | 173 ++++++++++++++++++++++++++++++++-
>
> First of all, I saw that all prior visitors should always have unit tests
> under tests/unit/. Maybe we should also attach an unit test?
>
Yep.
>> 2 files changed, 178 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/qapi/dealloc-visitor.h b/include/qapi/dealloc-visitor.h
>> index c36715fdf3..96c7bf35c3 100644
>> --- a/include/qapi/dealloc-visitor.h
>> +++ b/include/qapi/dealloc-visitor.h
>> @@ -25,4 +25,10 @@ typedef struct QapiDeallocVisitor QapiDeallocVisitor;
>> */
>> Visitor *qapi_dealloc_visitor_new(void);
>>
>> +/*
>> + * Like qapi_dealloc_visitor_new but visits a QObject and only frees
>> + * present members.
>> + */
>> +Visitor *qapi_dealloc_present_visitor_new(QObject *);
>> +
>> #endif
>> diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
>> index 57a2c904bb..90b017cc93 100644
>> --- a/qapi/qapi-dealloc-visitor.c
>> +++ b/qapi/qapi-dealloc-visitor.c
>> @@ -14,14 +14,146 @@
>>
>> #include "qemu/osdep.h"
>> #include "qapi/dealloc-visitor.h"
>> +#include "qemu/queue.h"
>> +#include "qobject/qdict.h"
>> +#include "qobject/qlist.h"
>> #include "qobject/qnull.h"
>> #include "qapi/visitor-impl.h"
>>
>> +typedef struct QStackEntry {
>> + QObject *obj; /* QDict or QList being visited */
>> + void *qapi;
>
> This one is only for debugging purpose, but not required, right?
>
> I wonder if we could just rely on the unit test for correctness, otherwise
> we kind of run some testing code with/without --enable-debug. Not a huge
> deal I think.. so see this a pure question.
>
I guess I can drop it. Should be ok.
>> + const QListEntry *entry; /* If @obj is QList: unvisited tail */
>> + QSLIST_ENTRY(QStackEntry) node;
>> +} QStackEntry;
>> +
>> struct QapiDeallocVisitor
>> {
>> Visitor visitor;
>> + QObject *root;
>> + QSLIST_HEAD(, QStackEntry) stack;
>> };
>>
>> +static void qapi_dealloc_pop(Visitor *v, void **obj)
>> +{
>> + QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
>> + QStackEntry *se = QSLIST_FIRST(&qdv->stack);
>> +
>> + assert(se && se->qapi == obj);
>> + QSLIST_REMOVE_HEAD(&qdv->stack, node);
>> + g_free(se);
>> +}
>> +
>> +static void qapi_dealloc_push(Visitor *v, QObject *obj, void *qapi)
>> +{
>> + QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
>> + QStackEntry *se = g_new0(QStackEntry, 1);
>> +
>> + assert(obj);
>> + se->obj = obj;
>> + se->qapi = qapi;
>> +
>> + if (qobject_type(obj) == QTYPE_QLIST) {
>> + se->entry = qlist_first(qobject_to(QList, obj));
>
> I still don't yet understand why do we care about lists here.
>
> I'm trying to guess, what this code wanted to do: we pushed the 1st entry
> of the list into the stack, trying to make it as a reference for all the
> items later within the list.
>
> But IIUC we can still define the conditiona-dealloc visitor to be even
> simpler, right? Say, if the ref qobject has the qlist object, then free
> the whole list?
>
I need to look at this closely, but I think we need to hold the list ref
so we can walk each of the objects inside of it and free them, then free
the empty list. When you "free the whole list" that would mean
qapi_free_<something>, which we can't do here, being already inside
visitor code; and free(list) doesn't free it's children, of course.
> IMHO there're three things we need to manage on conditional deallocations:
> (1) struct, (2) list, (3) alternatives. All the rest seem to be scalars.
>
> So I wonder if we could define this visitor, so that it treats both (2) and
> (3) the same way (to always dealloc as long as present).
>
> That sounds at least making more sense when I picture that in migration
> parameters: if we have a list in the parameters (e.g. cpr-exec-command), as
> long as the list is present in the ref, we should free the whole thing
> completely on the other one being visited.
>
Yes, that makes sense. I'm not expecting individual list items to be
ever updated separately. However, as I said above, I'm not sure whether
we can free the whole list without some sort of a walk of its
members. I'll check and get back to you.
>> + }
>> +
>> + QSLIST_INSERT_HEAD(&qdv->stack, se, node);
>> +}
>> +
>> +static QObject *qapi_dealloc_try_get_object(QapiDeallocVisitor *qdv, const char *name)
>> +{
>> + QStackEntry *se = QSLIST_FIRST(&qdv->stack);
>> + QObject *qobj;
>> + QObject *ret = NULL;
>> +
>> + if (!se) {
>> + assert(qdv->root);
>> + return qdv->root;
>> + }
>> +
>> + qobj = se->obj;
>> + assert(qobj);
>> +
>> + if (qobject_type(qobj) == QTYPE_QDICT) {
>> + assert(name);
>> + ret = qdict_get(qobject_to(QDict, qobj), name);
>> + } else {
>> + assert(qobject_type(qobj) == QTYPE_QLIST);
>> + assert(!name);
>> + if (se->entry) {
>> + ret = qlist_entry_obj(se->entry);
>> + }
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static bool qapi_dealloc_present_start_struct(Visitor *v, const char *name,
>> + void **obj, size_t size,
>> + Error **errp)
>> +{
>> + QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
>> + QObject *qobj = qapi_dealloc_try_get_object(qdv, name);
>> +
>> + if (!qobj) {
>> + return false;
>> + }
>> + assert(qobject_type(qobj) == QTYPE_QDICT);
>> + qapi_dealloc_push(v, qobj, obj);
>> + return true;
>> +}
>> +
>> +static void qapi_dealloc_present_end_struct(Visitor *v, void **obj)
>> +{
>> + QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
>> + QStackEntry *se = QSLIST_FIRST(&qdv->stack);
>> +
>> + assert(qobject_type(se->obj) == QTYPE_QDICT);
>> + qapi_dealloc_pop(v, obj);
>> +
>> + if (obj) {
>> + g_free(*obj);
>> + }
>> +}
>> +
>> +static bool qapi_dealloc_present_start_list(Visitor *v, const char *name,
>> + GenericList **list, size_t size,
>> + Error **errp)
>> +{
>> + QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
>> + QObject *qobj = qapi_dealloc_try_get_object(qdv, name);
>> +
>> + if (!qobj) {
>> + return false;
>> + }
>> + assert(qobject_type(qobj) == QTYPE_QLIST);
>> + qapi_dealloc_push(v, qobj, list);
>> + return true;
>> +}
>> +
>> +static void qapi_dealloc_present_end_list(Visitor *v, void **obj)
>> +{
>> + QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
>> + QStackEntry *se = QSLIST_FIRST(&qdv->stack);
>> +
>> + assert(qobject_type(se->obj) == QTYPE_QLIST);
>> + qapi_dealloc_pop(v, obj);
>> +}
>> +
>> +static void qapi_dealloc_present_free(Visitor *v)
>> +{
>> + QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
>> +
>> + while (!QSLIST_EMPTY(&qdv->stack)) {
>> + QStackEntry *se = QSLIST_FIRST(&qdv->stack);
>> +
>> + QSLIST_REMOVE_HEAD(&qdv->stack, node);
>> + g_free(se);
>> + }
>> + qobject_unref(qdv->root);
>> + g_free(qdv);
>> +}
>> +
>> static bool qapi_dealloc_start_struct(Visitor *v, const char *name, void **obj,
>> size_t unused, Error **errp)
>> {
>> @@ -35,6 +167,21 @@ static void qapi_dealloc_end_struct(Visitor *v, void **obj)
>> }
>> }
>>
>> +static bool qapi_dealloc_start_alternate(Visitor *v, const char *name,
>> + GenericAlternate **obj, size_t size,
>> + Error **errp)
>> +{
>> + QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
>> + QObject *qobj = qapi_dealloc_try_get_object(qdv, name);
>> +
>> + if (!qobj) {
>> + return false;
>> + }
>> + assert(*obj);
>> + (*obj)->type = qobject_type(qobj);
>> + return true;
>> +}
>> +
>> static void qapi_dealloc_end_alternate(Visitor *v, void **obj)
>> {
>> if (obj) {
>> @@ -117,13 +264,14 @@ static void qapi_dealloc_free(Visitor *v)
>> g_free(container_of(v, QapiDeallocVisitor, visitor));
>> }
>>
>> -Visitor *qapi_dealloc_visitor_new(void)
>> +static QapiDeallocVisitor *qapi_dealloc_visitor_new_base(void)
>> {
>> QapiDeallocVisitor *v;
>>
>> v = g_malloc0(sizeof(*v));
>>
>> v->visitor.type = VISITOR_DEALLOC;
>> +
>> v->visitor.start_struct = qapi_dealloc_start_struct;
>> v->visitor.end_struct = qapi_dealloc_end_struct;
>> v->visitor.end_alternate = qapi_dealloc_end_alternate;
>> @@ -139,5 +287,28 @@ Visitor *qapi_dealloc_visitor_new(void)
>> v->visitor.type_null = qapi_dealloc_type_null;
>> v->visitor.free = qapi_dealloc_free;
>
> IMHO, setting the hooks once then overwrite, is less clean than moving them
> into qapi_dealloc_visitor_new() if dealloc_present visitor will do that.
>
ok
>>
>> + return v;
>> +}
>> +
>> +Visitor *qapi_dealloc_visitor_new(void)
>> +{
>> + QapiDeallocVisitor *v = qapi_dealloc_visitor_new_base();
>> +
>> + return &v->visitor;
>> +}
>> +
>> +Visitor *qapi_dealloc_present_visitor_new(QObject *obj)
>> +{
>> + QapiDeallocVisitor *v = qapi_dealloc_visitor_new_base();
>> +
>> + v->visitor.start_alternate = qapi_dealloc_start_alternate;
>> + v->visitor.start_list = qapi_dealloc_present_start_list;
>> + v->visitor.end_list = qapi_dealloc_present_end_list;
>> + v->visitor.start_struct = qapi_dealloc_present_start_struct;
>> + v->visitor.end_struct = qapi_dealloc_present_end_struct;
>> + v->visitor.free = qapi_dealloc_present_free;
>> +
>> + v->root = qobject_ref(obj);
>> +
>> return &v->visitor;
>> }
>> --
>> 2.51.0
>>
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v2 4/9] qapi: Implement qapi_dealloc_present_visitor
2026-02-06 12:19 ` Fabiano Rosas
@ 2026-02-06 17:38 ` Peter Xu
0 siblings, 0 replies; 28+ messages in thread
From: Peter Xu @ 2026-02-06 17:38 UTC (permalink / raw)
To: Fabiano Rosas; +Cc: qemu-devel, armbru, ppandit, Michael Roth
On Fri, Feb 06, 2026 at 09:19:21AM -0300, Fabiano Rosas wrote:
> >> +static void qapi_dealloc_push(Visitor *v, QObject *obj, void *qapi)
> >> +{
> >> + QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
> >> + QStackEntry *se = g_new0(QStackEntry, 1);
> >> +
> >> + assert(obj);
> >> + se->obj = obj;
> >> + se->qapi = qapi;
> >> +
> >> + if (qobject_type(obj) == QTYPE_QLIST) {
> >> + se->entry = qlist_first(qobject_to(QList, obj));
> >
> > I still don't yet understand why do we care about lists here.
> >
> > I'm trying to guess, what this code wanted to do: we pushed the 1st entry
> > of the list into the stack, trying to make it as a reference for all the
> > items later within the list.
> >
> > But IIUC we can still define the conditiona-dealloc visitor to be even
> > simpler, right? Say, if the ref qobject has the qlist object, then free
> > the whole list?
> >
>
> I need to look at this closely, but I think we need to hold the list ref
> so we can walk each of the objects inside of it and free them, then free
> the empty list. When you "free the whole list" that would mean
> qapi_free_<something>, which we can't do here, being already inside
> visitor code; and free(list) doesn't free it's children, of course.
True.
>
> > IMHO there're three things we need to manage on conditional deallocations:
> > (1) struct, (2) list, (3) alternatives. All the rest seem to be scalars.
> >
> > So I wonder if we could define this visitor, so that it treats both (2) and
> > (3) the same way (to always dealloc as long as present).
> >
> > That sounds at least making more sense when I picture that in migration
> > parameters: if we have a list in the parameters (e.g. cpr-exec-command), as
> > long as the list is present in the ref, we should free the whole thing
> > completely on the other one being visited.
> >
>
> Yes, that makes sense. I'm not expecting individual list items to be
> ever updated separately. However, as I said above, I'm not sure whether
> we can free the whole list without some sort of a walk of its
> members. I'll check and get back to you.
My QAPI kongfu is very limited.. but my current understanding is the
visitor framework will make sure everything will be visited properly within
the list, and after each visit to the list entry, it'll finally free the
list structure (in case of dealloc visitor) via qapi_dealloc_next_list().
I believe that's also what the new dealloc_present visitor should rely on.
So my understanding is maybe we don't need to hold any reference to the
list being walked. However maybe we want to remember we're walking this
list, then no matter how deep we walk into this list, we should stop
checking / referencing the ref object but just always free everything (aka,
fallback to the basic dealloc operations).
When thinking about that, I found maybe it's not that list is special; it's
when the "top level" is special.
Consider if we have a migration parameter that is a nested struct:
- struct1
- struct2
- int
- struct3
- boolean
Then if currently we have this value for this parameter:
- struct1
- struct2
- int=1
- struct3
- string="foobar"
When we take an update from the user on this specific parameter, and if the
user's input is:
- struct1
- struct2
- int=2
(omit struct3)
With your current patch, you'll leave struct3 and the sub-field string
untouched. However, IIUC the current semantics (of migrate_set_parameters,
at least) should be that we use the new data to fully replace the old
struct1 tree, making sure struct3 alone with the string to be unset?
So I wonder if we should just remember the top of the stack (which must be
a qdict), then free anything below that whenever the top element is present
in the ref.
--
Peter Xu
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 5/9] qapi: Add QAPI_MERGE
2026-02-02 22:40 [PATCH v2 0/9] qapi: Use visitors for migration parameters handling Fabiano Rosas
` (3 preceding siblings ...)
2026-02-02 22:40 ` [PATCH v2 4/9] qapi: Implement qapi_dealloc_present_visitor Fabiano Rosas
@ 2026-02-02 22:40 ` Fabiano Rosas
2026-02-13 14:57 ` Markus Armbruster
2026-02-02 22:40 ` [PATCH v2 6/9] migration/options: Use QAPI_MERGE in migrate_params_test_apply Fabiano Rosas
` (4 subsequent siblings)
9 siblings, 1 reply; 28+ messages in thread
From: Fabiano Rosas @ 2026-02-02 22:40 UTC (permalink / raw)
To: qemu-devel; +Cc: peterx, armbru, ppandit, Michael Roth
The migration subsystem currently has code to merge two objects of the
same type. It does so by checking which fields are present in a source
object and overwriting the corresponding fields on the destination
object. This leads to a lot of open-coded lines such as:
if (src->has_foobar) {
dst->foobar = src->foobar;
}
This pattern could be replaced by a copy using visitors. Implement a
macro that extracts elements from a source object using an output
visitor and merges it with a destination object using an input
visitor.
Acked-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
include/qapi/type-helpers.h | 40 +++++++++++++++++++++++++++++++++++++
1 file changed, 40 insertions(+)
diff --git a/include/qapi/type-helpers.h b/include/qapi/type-helpers.h
index fc8352cdec..f22d7b7139 100644
--- a/include/qapi/type-helpers.h
+++ b/include/qapi/type-helpers.h
@@ -10,6 +10,9 @@
*/
#include "qapi/qapi-types-common.h"
+#include "qapi/qobject-input-visitor.h"
+#include "qapi/qobject-output-visitor.h"
+#include "qapi/dealloc-visitor.h"
HumanReadableText *human_readable_text_from_str(GString *str);
@@ -20,3 +23,40 @@ HumanReadableText *human_readable_text_from_str(GString *str);
* cleanup.
*/
char **strv_from_str_list(const strList *list);
+
+/*
+ * Merge @src over @dst by copying deep clones of the present members
+ * from @src to @dst. Non-present on @src are left untouched on
+ * @dst. Pointer members that are overwritten are also freed.
+ */
+#define QAPI_MERGE(type, dst_, src_) \
+ ({ \
+ QObject *out_ = NULL; \
+ Visitor *v_; \
+ /* read in from src */ \
+ v_ = qobject_output_visitor_new(&out_); \
+ visit_type_ ## type(v_, NULL, &src_, &error_abort); \
+ visit_complete(v_, &out_); \
+ visit_free(v_); \
+ /* \
+ * Free the memory for which the pointers will be overwritten \
+ * in the next step. \
+ */ \
+ v_ = qapi_dealloc_present_visitor_new(out_); \
+ visit_start_struct(v_, NULL, NULL, 0, &error_abort); \
+ visit_type_ ## type ## _members(v_, dst_, &error_abort); \
+ visit_end_struct(v_, NULL); \
+ visit_free(v_); \
+ /* \
+ * Write to dst but leave existing fields intact (except for \
+ * has_* which will be updated according to their presence in \
+ * src). \
+ */ \
+ v_ = qobject_input_visitor_new(out_); \
+ visit_start_struct(v_, NULL, NULL, 0, &error_abort); \
+ visit_type_ ## type ## _members(v_, dst_, &error_abort); \
+ visit_check_struct(v_, &error_abort); \
+ visit_end_struct(v_, NULL); \
+ visit_free(v_); \
+ qobject_unref(out_); \
+ })
--
2.51.0
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH v2 5/9] qapi: Add QAPI_MERGE
2026-02-02 22:40 ` [PATCH v2 5/9] qapi: Add QAPI_MERGE Fabiano Rosas
@ 2026-02-13 14:57 ` Markus Armbruster
0 siblings, 0 replies; 28+ messages in thread
From: Markus Armbruster @ 2026-02-13 14:57 UTC (permalink / raw)
To: Fabiano Rosas; +Cc: qemu-devel, peterx, ppandit, Michael Roth
Fabiano Rosas <farosas@suse.de> writes:
> The migration subsystem currently has code to merge two objects of the
> same type. It does so by checking which fields are present in a source
> object and overwriting the corresponding fields on the destination
> object. This leads to a lot of open-coded lines such as:
>
> if (src->has_foobar) {
> dst->foobar = src->foobar;
> }
It does this to update configuration. Both the configuration and the
update is a struct MigrationParameters. To enable update, not just
complete overwrite, the struct's members are all optional. If update's
member is present, it replaces the configuration's member.
Note that this is a *shallow* operation. There's no recursion.
MigrationParameters is almost flat: two members are arrays. These
members are updated wholesale, i.e. an array is either not touched or
replaced completely. We don't look at array elements.
> This pattern could be replaced by a copy using visitors. Implement a
> macro that extracts elements from a source object using an output
> visitor and merges it with a destination object using an input
> visitor.
>
> Acked-by: Peter Xu <peterx@redhat.com>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
> include/qapi/type-helpers.h | 40 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 40 insertions(+)
>
> diff --git a/include/qapi/type-helpers.h b/include/qapi/type-helpers.h
> index fc8352cdec..f22d7b7139 100644
> --- a/include/qapi/type-helpers.h
> +++ b/include/qapi/type-helpers.h
> @@ -10,6 +10,9 @@
> */
>
> #include "qapi/qapi-types-common.h"
> +#include "qapi/qobject-input-visitor.h"
> +#include "qapi/qobject-output-visitor.h"
> +#include "qapi/dealloc-visitor.h"
>
> HumanReadableText *human_readable_text_from_str(GString *str);
>
> @@ -20,3 +23,40 @@ HumanReadableText *human_readable_text_from_str(GString *str);
> * cleanup.
> */
> char **strv_from_str_list(const strList *list);
> +
> +/*
> + * Merge @src over @dst by copying deep clones of the present members
> + * from @src to @dst. Non-present on @src are left untouched on
> + * @dst. Pointer members that are overwritten are also freed.
> + */
> +#define QAPI_MERGE(type, dst_, src_) \
The comment uses @dst and @src, the code uses dst_ and src_. Make them
consistent, please.
> + ({ \
> + QObject *out_ = NULL; \
> + Visitor *v_; \
> + /* read in from src */ \
> + v_ = qobject_output_visitor_new(&out_); \
> + visit_type_ ## type(v_, NULL, &src_, &error_abort); \
> + visit_complete(v_, &out_); \
> + visit_free(v_); \
This serializes @src_ to @out_.
> + /* \
> + * Free the memory for which the pointers will be overwritten \
> + * in the next step. \
> + */ \
> + v_ = qapi_dealloc_present_visitor_new(out_); \
> + visit_start_struct(v_, NULL, NULL, 0, &error_abort); \
> + visit_type_ ## type ## _members(v_, dst_, &error_abort); \
> + visit_end_struct(v_, NULL); \
> + visit_free(v_); \
This frees members of @dst_ that are about to be overwritten, using the
dealloc present visitor from the previous patch. Which I haven't
reviewed, yet.
> + /* \
> + * Write to dst but leave existing fields intact (except for \
> + * has_* which will be updated according to their presence in \
> + * src). \
> + */ \
> + v_ = qobject_input_visitor_new(out_); \
> + visit_start_struct(v_, NULL, NULL, 0, &error_abort); \
> + visit_type_ ## type ## _members(v_, dst_, &error_abort); \
> + visit_check_struct(v_, &error_abort); \
> + visit_end_struct(v_, NULL); \
> + visit_free(v_); \
This deserializes @out_ into @dst_, but in an unusual way. The usual
way would be like
visit_start_struct(v, name, (void **)&dst_, sizeof(type),
&error_abort);
visit_type_ ## type ## _members(v, dst_, &error_abort);
visit_check_struct(v, &error_abort);
visit_end_struct(v, (void **)&dst_);
This is code from generated visit_type_TYPE() with @obj replaced by
&dst_, @errp replaced by &error_abort, and handling now impossible
errors deleted.
There's a a small difference that doesn't actually matter:
visit_start_struct() argument @name. visit_start_struct() ignores this
argument when visiting the root of a tree (again, see the big comment in
qapi/visitor.h).
The difference that matters follows.
The usual way passes &dst_ to visit_start_struct(), visit_end_struct(),
and dst_ to visit_type_TYPE_members().
The other usual way passes NULL to all three. That's a virtual walk.
See also the big comment in qapi/visitor.h.
Your QAPI_MERGE() mixes the two. Hmm.
Input visitors create new objects / work on zero-initialized objects:
visit_type_TYPE_members() with an input visitor takes a TYPE *obj
pointing to zero-initialized memory.
QAPI_MERGE() does not zero-initialize the memory it passes to the input
visitor. It merely frees objects the input visitor is going to
overwrite.
Memory the input visitor doesn't overwrite remains as it is. As long as
the input visitor overwrite exactly all the member memory for each
member it actually visits, this results in a merge operation.
At this point, I have a queasy feeling in my stomach: we're upending
design assumptions. I can't point to anything wrong, but this is risky
business.
Another observation: visitors are deep, not shallow. What happens when
@src isn't flat? Say it contains an array member. The input visitor
will first replace @dst's array wholesale by @src's. Then it'll duly
recurse into the array, and replace each of its members by itself. I
think. Does that work? I hope. Is it weird and surprising? Yes!
> + qobject_unref(out_); \
> + })
Could we do this in a simpler and cleaner way?
In handwritten code, we'd simply walk source and destination
simultaneously. The visitor infrastructure doesn't let us do that; it
fundamentally walks a single object. Perhaps we could somehow cajole it
into letting us do it. Can't tell offhand.
We could have an input visitor that updates an existing object. No need
for a dealloc present visitor then. It should take care to avoid the
recursive update I called weird and surprising.
We could serialize both objects to be merged, shallowly merge the
resulting QDicts, deserialize. One more serialize, which is expensive,
but if two expensive expensive serialize / deserialize is fine, three
should be fine, too. No need for a dealloc present visitor then.
Thoughts?
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 6/9] migration/options: Use QAPI_MERGE in migrate_params_test_apply
2026-02-02 22:40 [PATCH v2 0/9] qapi: Use visitors for migration parameters handling Fabiano Rosas
` (4 preceding siblings ...)
2026-02-02 22:40 ` [PATCH v2 5/9] qapi: Add QAPI_MERGE Fabiano Rosas
@ 2026-02-02 22:40 ` Fabiano Rosas
2026-02-02 22:40 ` [PATCH v2 7/9] migration/options: Open code migrate_params_apply Fabiano Rosas
` (3 subsequent siblings)
9 siblings, 0 replies; 28+ messages in thread
From: Fabiano Rosas @ 2026-02-02 22:40 UTC (permalink / raw)
To: qemu-devel; +Cc: peterx, armbru, ppandit
Convert the code in migrate_params_test_apply() from an open-coded
copy of every migration parameter to a copy using visitors. The
current code has conditionals for each parameter's has_* field, which
is exactly what the visitors do.
This hides the details of QAPI from the migration code and avoids the
need to update migrate_params_test_apply() every time a new migration
parameter is added. Both were very confusing and while the visitor
code can become a bit involved, there is no need for new contributors
to ever touch it.
Move the QAPI_CLONE_MEMBERS into the caller, so QAPI_CLONE can be used
and there's no need to allocate memory in the migration
code. Similarly, turn 'tmp' into a pointer so the proper qapi_free_
routine can be used.
An extra call to migrate_mark_all_params_present() is now needed
because the visitors update the has_ field for non-present fields, but
we actually want them all set so QAPI_CLONE_MEMBERS copies all of
them.
Reviewed-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
migration/options.c | 145 +++-----------------------------------------
1 file changed, 10 insertions(+), 135 deletions(-)
diff --git a/migration/options.c b/migration/options.c
index 3c80cdfd3a..180ebed51c 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -19,6 +19,7 @@
#include "qapi/qapi-commands-migration.h"
#include "qapi/qapi-visit-migration.h"
#include "qapi/qmp/qerror.h"
+#include "qapi/type-helpers.h"
#include "qobject/qnull.h"
#include "system/runstate.h"
#include "migration/colo.h"
@@ -1265,133 +1266,6 @@ bool migrate_params_check(MigrationParameters *params, Error **errp)
return true;
}
-static void migrate_params_test_apply(MigrationParameters *params,
- MigrationParameters *dest)
-{
- MigrationState *s = migrate_get_current();
-
- QAPI_CLONE_MEMBERS(MigrationParameters, dest, &s->parameters);
-
- if (params->has_throttle_trigger_threshold) {
- dest->throttle_trigger_threshold = params->throttle_trigger_threshold;
- }
-
- if (params->has_cpu_throttle_initial) {
- dest->cpu_throttle_initial = params->cpu_throttle_initial;
- }
-
- if (params->has_cpu_throttle_increment) {
- dest->cpu_throttle_increment = params->cpu_throttle_increment;
- }
-
- if (params->has_cpu_throttle_tailslow) {
- dest->cpu_throttle_tailslow = params->cpu_throttle_tailslow;
- }
-
- if (params->tls_creds) {
- qapi_free_StrOrNull(dest->tls_creds);
- dest->tls_creds = QAPI_CLONE(StrOrNull, params->tls_creds);
- }
-
- if (params->tls_hostname) {
- qapi_free_StrOrNull(dest->tls_hostname);
- dest->tls_hostname = QAPI_CLONE(StrOrNull, params->tls_hostname);
- }
-
- if (params->tls_authz) {
- qapi_free_StrOrNull(dest->tls_authz);
- dest->tls_authz = QAPI_CLONE(StrOrNull, params->tls_authz);
- }
-
- if (params->has_max_bandwidth) {
- dest->max_bandwidth = params->max_bandwidth;
- }
-
- if (params->has_avail_switchover_bandwidth) {
- dest->avail_switchover_bandwidth = params->avail_switchover_bandwidth;
- }
-
- if (params->has_downtime_limit) {
- dest->downtime_limit = params->downtime_limit;
- }
-
- if (params->has_x_checkpoint_delay) {
- dest->x_checkpoint_delay = params->x_checkpoint_delay;
- }
-
- if (params->has_multifd_channels) {
- dest->multifd_channels = params->multifd_channels;
- }
- if (params->has_multifd_compression) {
- dest->multifd_compression = params->multifd_compression;
- }
- if (params->has_multifd_qatzip_level) {
- dest->multifd_qatzip_level = params->multifd_qatzip_level;
- }
- if (params->has_multifd_zlib_level) {
- dest->multifd_zlib_level = params->multifd_zlib_level;
- }
- if (params->has_multifd_zstd_level) {
- dest->multifd_zstd_level = params->multifd_zstd_level;
- }
- if (params->has_xbzrle_cache_size) {
- dest->xbzrle_cache_size = params->xbzrle_cache_size;
- }
- if (params->has_max_postcopy_bandwidth) {
- dest->max_postcopy_bandwidth = params->max_postcopy_bandwidth;
- }
- if (params->has_max_cpu_throttle) {
- dest->max_cpu_throttle = params->max_cpu_throttle;
- }
- if (params->has_announce_initial) {
- dest->announce_initial = params->announce_initial;
- }
- if (params->has_announce_max) {
- dest->announce_max = params->announce_max;
- }
- if (params->has_announce_rounds) {
- dest->announce_rounds = params->announce_rounds;
- }
- if (params->has_announce_step) {
- dest->announce_step = params->announce_step;
- }
-
- if (params->has_block_bitmap_mapping) {
- qapi_free_BitmapMigrationNodeAliasList(dest->block_bitmap_mapping);
- dest->block_bitmap_mapping = QAPI_CLONE(BitmapMigrationNodeAliasList,
- params->block_bitmap_mapping);
- }
-
- if (params->has_x_vcpu_dirty_limit_period) {
- dest->x_vcpu_dirty_limit_period =
- params->x_vcpu_dirty_limit_period;
- }
- if (params->has_vcpu_dirty_limit) {
- dest->vcpu_dirty_limit = params->vcpu_dirty_limit;
- }
-
- if (params->has_mode) {
- dest->mode = params->mode;
- }
-
- if (params->has_zero_page_detection) {
- dest->zero_page_detection = params->zero_page_detection;
- }
-
- if (params->has_direct_io) {
- dest->direct_io = params->direct_io;
- }
-
- if (params->has_cpr_exec_command) {
- qapi_free_strList(dest->cpr_exec_command);
- dest->cpr_exec_command = QAPI_CLONE(strList, params->cpr_exec_command);
- }
-}
-
-/*
- * Caller must ensure the has_* fields of @params are true so they all
- * get copied and the pointer members don't dangle.
- */
static void migrate_params_apply(MigrationParameters *params)
{
MigrationState *s = migrate_get_current();
@@ -1400,12 +1274,17 @@ static void migrate_params_apply(MigrationParameters *params)
migrate_tls_opts_free(cur);
qapi_free_BitmapMigrationNodeAliasList(cur->block_bitmap_mapping);
qapi_free_strList(cur->cpr_exec_command);
+
+ /* mark all present, so they're all copied */
+ migrate_mark_all_params_present(params);
QAPI_CLONE_MEMBERS(MigrationParameters, cur, params);
}
void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
{
- MigrationParameters tmp;
+ MigrationState *s = migrate_get_current();
+ g_autoptr(MigrationParameters) tmp = QAPI_CLONE(MigrationParameters,
+ &s->parameters);
/*
* Convert QTYPE_QNULL and NULL to the empty string (""). Even
@@ -1419,14 +1298,10 @@ void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
tls_opt_to_str(params->tls_hostname);
tls_opt_to_str(params->tls_authz);
- migrate_params_test_apply(params, &tmp);
+ QAPI_MERGE(MigrationParameters, tmp, params);
- if (migrate_params_check(&tmp, errp)) {
- migrate_params_apply(&tmp);
+ if (migrate_params_check(tmp, errp)) {
+ migrate_params_apply(tmp);
migrate_post_update_params(params, errp);
}
-
- migrate_tls_opts_free(&tmp);
- qapi_free_BitmapMigrationNodeAliasList(tmp.block_bitmap_mapping);
- qapi_free_strList(tmp.cpr_exec_command);
}
--
2.51.0
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH v2 7/9] migration/options: Open code migrate_params_apply
2026-02-02 22:40 [PATCH v2 0/9] qapi: Use visitors for migration parameters handling Fabiano Rosas
` (5 preceding siblings ...)
2026-02-02 22:40 ` [PATCH v2 6/9] migration/options: Use QAPI_MERGE in migrate_params_test_apply Fabiano Rosas
@ 2026-02-02 22:40 ` Fabiano Rosas
2026-02-05 22:14 ` Peter Xu
2026-02-02 22:41 ` [PATCH v2 8/9] migration/options: Stop freeing s->parameters members individually Fabiano Rosas
` (2 subsequent siblings)
9 siblings, 1 reply; 28+ messages in thread
From: Fabiano Rosas @ 2026-02-02 22:40 UTC (permalink / raw)
To: qemu-devel; +Cc: peterx, armbru, ppandit
Remove migrate_params_apply so the logic of setting migration
parameters is all in one spot.
Suggested-by: Prasad Pandit <ppandit@redhat.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
migration/options.c | 33 ++++++++++++++-------------------
1 file changed, 14 insertions(+), 19 deletions(-)
diff --git a/migration/options.c b/migration/options.c
index 180ebed51c..733aae51a8 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -1266,25 +1266,11 @@ bool migrate_params_check(MigrationParameters *params, Error **errp)
return true;
}
-static void migrate_params_apply(MigrationParameters *params)
-{
- MigrationState *s = migrate_get_current();
- MigrationParameters *cur = &s->parameters;
-
- migrate_tls_opts_free(cur);
- qapi_free_BitmapMigrationNodeAliasList(cur->block_bitmap_mapping);
- qapi_free_strList(cur->cpr_exec_command);
-
- /* mark all present, so they're all copied */
- migrate_mark_all_params_present(params);
- QAPI_CLONE_MEMBERS(MigrationParameters, cur, params);
-}
-
void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
{
MigrationState *s = migrate_get_current();
- g_autoptr(MigrationParameters) tmp = QAPI_CLONE(MigrationParameters,
- &s->parameters);
+ MigrationParameters *cur = &s->parameters;
+ g_autoptr(MigrationParameters) tmp = QAPI_CLONE(MigrationParameters, cur);
/*
* Convert QTYPE_QNULL and NULL to the empty string (""). Even
@@ -1300,8 +1286,17 @@ void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
QAPI_MERGE(MigrationParameters, tmp, params);
- if (migrate_params_check(tmp, errp)) {
- migrate_params_apply(tmp);
- migrate_post_update_params(params, errp);
+ if (!migrate_params_check(tmp, errp)) {
+ return;
}
+
+ migrate_tls_opts_free(cur);
+ qapi_free_BitmapMigrationNodeAliasList(cur->block_bitmap_mapping);
+ qapi_free_strList(cur->cpr_exec_command);
+
+ /* mark all present, so they're all copied */
+ migrate_mark_all_params_present(tmp);
+ QAPI_CLONE_MEMBERS(MigrationParameters, cur, tmp);
+
+ migrate_post_update_params(params, errp);
}
--
2.51.0
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH v2 7/9] migration/options: Open code migrate_params_apply
2026-02-02 22:40 ` [PATCH v2 7/9] migration/options: Open code migrate_params_apply Fabiano Rosas
@ 2026-02-05 22:14 ` Peter Xu
2026-02-06 12:25 ` Fabiano Rosas
0 siblings, 1 reply; 28+ messages in thread
From: Peter Xu @ 2026-02-05 22:14 UTC (permalink / raw)
To: Fabiano Rosas; +Cc: qemu-devel, armbru, ppandit
On Mon, Feb 02, 2026 at 07:40:59PM -0300, Fabiano Rosas wrote:
> Remove migrate_params_apply so the logic of setting migration
> parameters is all in one spot.
>
> Suggested-by: Prasad Pandit <ppandit@redhat.com>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
> migration/options.c | 33 ++++++++++++++-------------------
> 1 file changed, 14 insertions(+), 19 deletions(-)
>
> diff --git a/migration/options.c b/migration/options.c
> index 180ebed51c..733aae51a8 100644
> --- a/migration/options.c
> +++ b/migration/options.c
> @@ -1266,25 +1266,11 @@ bool migrate_params_check(MigrationParameters *params, Error **errp)
> return true;
> }
>
> -static void migrate_params_apply(MigrationParameters *params)
> -{
> - MigrationState *s = migrate_get_current();
> - MigrationParameters *cur = &s->parameters;
> -
> - migrate_tls_opts_free(cur);
> - qapi_free_BitmapMigrationNodeAliasList(cur->block_bitmap_mapping);
> - qapi_free_strList(cur->cpr_exec_command);
> -
> - /* mark all present, so they're all copied */
> - migrate_mark_all_params_present(params);
> - QAPI_CLONE_MEMBERS(MigrationParameters, cur, params);
> -}
I actually like these smaller functions when their name can represent what
it does (so I read less but know what it is doing faster).. But yeah if
both of you like to drop it, I'm OK.
Reviewed-by: Peter Xu <peterx@redhat.com>
--
Peter Xu
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v2 7/9] migration/options: Open code migrate_params_apply
2026-02-05 22:14 ` Peter Xu
@ 2026-02-06 12:25 ` Fabiano Rosas
0 siblings, 0 replies; 28+ messages in thread
From: Fabiano Rosas @ 2026-02-06 12:25 UTC (permalink / raw)
To: Peter Xu; +Cc: qemu-devel, armbru, ppandit
Peter Xu <peterx@redhat.com> writes:
> On Mon, Feb 02, 2026 at 07:40:59PM -0300, Fabiano Rosas wrote:
>> Remove migrate_params_apply so the logic of setting migration
>> parameters is all in one spot.
>>
>> Suggested-by: Prasad Pandit <ppandit@redhat.com>
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>> migration/options.c | 33 ++++++++++++++-------------------
>> 1 file changed, 14 insertions(+), 19 deletions(-)
>>
>> diff --git a/migration/options.c b/migration/options.c
>> index 180ebed51c..733aae51a8 100644
>> --- a/migration/options.c
>> +++ b/migration/options.c
>> @@ -1266,25 +1266,11 @@ bool migrate_params_check(MigrationParameters *params, Error **errp)
>> return true;
>> }
>>
>> -static void migrate_params_apply(MigrationParameters *params)
>> -{
>> - MigrationState *s = migrate_get_current();
>> - MigrationParameters *cur = &s->parameters;
>> -
>> - migrate_tls_opts_free(cur);
>> - qapi_free_BitmapMigrationNodeAliasList(cur->block_bitmap_mapping);
>> - qapi_free_strList(cur->cpr_exec_command);
>> -
>> - /* mark all present, so they're all copied */
>> - migrate_mark_all_params_present(params);
>> - QAPI_CLONE_MEMBERS(MigrationParameters, cur, params);
>> -}
>
> I actually like these smaller functions when their name can represent what
> it does (so I read less but know what it is doing faster).. But yeah if
> both of you like to drop it, I'm OK.
>
Yeah, I tend to agree with you. It's not a big deal anyway.
I want to eventually add a QMP-specific file to migration/ and move all
of the QMP command handlers in there. Having this all in one function
will be (_very slightly_) simpler to move later on.
> Reviewed-by: Peter Xu <peterx@redhat.com>
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 8/9] migration/options: Stop freeing s->parameters members individually
2026-02-02 22:40 [PATCH v2 0/9] qapi: Use visitors for migration parameters handling Fabiano Rosas
` (6 preceding siblings ...)
2026-02-02 22:40 ` [PATCH v2 7/9] migration/options: Open code migrate_params_apply Fabiano Rosas
@ 2026-02-02 22:41 ` Fabiano Rosas
2026-02-05 22:16 ` Peter Xu
2026-02-02 22:41 ` [PATCH v2 9/9] migration: Use migrate_params_free during finalize Fabiano Rosas
2026-02-05 22:05 ` [PATCH v2 0/9] qapi: Use visitors for migration parameters handling Peter Xu
9 siblings, 1 reply; 28+ messages in thread
From: Fabiano Rosas @ 2026-02-02 22:41 UTC (permalink / raw)
To: qemu-devel; +Cc: peterx, armbru, ppandit
Expecting every pointer member from s->parameters to be freed
individually is prone to leave some fields forgotten when the code is
eventually updated. Use a dealloc visitor to free them all at once.
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
migration/options.c | 24 +++++++++++++++++-------
1 file changed, 17 insertions(+), 7 deletions(-)
diff --git a/migration/options.c b/migration/options.c
index 733aae51a8..cc5a66750c 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -1002,11 +1002,21 @@ AnnounceParameters *migrate_announce_params(void)
return ≈
}
-static void migrate_tls_opts_free(MigrationParameters *params)
+static bool migrate_params_free(MigrationParameters *params, Error **errp)
{
- qapi_free_StrOrNull(params->tls_creds);
- qapi_free_StrOrNull(params->tls_hostname);
- qapi_free_StrOrNull(params->tls_authz);
+ Visitor *v = qapi_dealloc_visitor_new();
+ bool ret;
+
+ /*
+ * qapi_free_MigrationParameters can't be used here because
+ * MigrationParameters is embedded in MigrationState due to qdev
+ * needing to access the offset of the migration properties inside
+ * the migration object.
+ */
+ ret = visit_type_MigrationParameters_members(v, params, errp);
+ visit_free(v);
+
+ return ret;
}
/* normalize QTYPE_QNULL to QTYPE_QSTRING "" */
@@ -1290,9 +1300,9 @@ void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
return;
}
- migrate_tls_opts_free(cur);
- qapi_free_BitmapMigrationNodeAliasList(cur->block_bitmap_mapping);
- qapi_free_strList(cur->cpr_exec_command);
+ if (!migrate_params_free(cur, errp)) {
+ return;
+ }
/* mark all present, so they're all copied */
migrate_mark_all_params_present(tmp);
--
2.51.0
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH v2 8/9] migration/options: Stop freeing s->parameters members individually
2026-02-02 22:41 ` [PATCH v2 8/9] migration/options: Stop freeing s->parameters members individually Fabiano Rosas
@ 2026-02-05 22:16 ` Peter Xu
2026-02-06 12:29 ` Fabiano Rosas
0 siblings, 1 reply; 28+ messages in thread
From: Peter Xu @ 2026-02-05 22:16 UTC (permalink / raw)
To: Fabiano Rosas; +Cc: qemu-devel, armbru, ppandit
On Mon, Feb 02, 2026 at 07:41:00PM -0300, Fabiano Rosas wrote:
> Expecting every pointer member from s->parameters to be freed
> individually is prone to leave some fields forgotten when the code is
> eventually updated. Use a dealloc visitor to free them all at once.
>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
> migration/options.c | 24 +++++++++++++++++-------
> 1 file changed, 17 insertions(+), 7 deletions(-)
>
> diff --git a/migration/options.c b/migration/options.c
> index 733aae51a8..cc5a66750c 100644
> --- a/migration/options.c
> +++ b/migration/options.c
> @@ -1002,11 +1002,21 @@ AnnounceParameters *migrate_announce_params(void)
> return ≈
> }
>
> -static void migrate_tls_opts_free(MigrationParameters *params)
> +static bool migrate_params_free(MigrationParameters *params, Error **errp)
> {
> - qapi_free_StrOrNull(params->tls_creds);
> - qapi_free_StrOrNull(params->tls_hostname);
> - qapi_free_StrOrNull(params->tls_authz);
> + Visitor *v = qapi_dealloc_visitor_new();
> + bool ret;
> +
> + /*
> + * qapi_free_MigrationParameters can't be used here because
> + * MigrationParameters is embedded in MigrationState due to qdev
> + * needing to access the offset of the migration properties inside
> + * the migration object.
> + */
Ohhhhhhhh.... thanks for the comment!
Reviewed-by: Peter Xu <peterx@redhat.com>
> + ret = visit_type_MigrationParameters_members(v, params, errp);
> + visit_free(v);
> +
> + return ret;
> }
--
Peter Xu
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v2 8/9] migration/options: Stop freeing s->parameters members individually
2026-02-05 22:16 ` Peter Xu
@ 2026-02-06 12:29 ` Fabiano Rosas
2026-02-06 16:20 ` Peter Xu
0 siblings, 1 reply; 28+ messages in thread
From: Fabiano Rosas @ 2026-02-06 12:29 UTC (permalink / raw)
To: Peter Xu; +Cc: qemu-devel, armbru, ppandit
Peter Xu <peterx@redhat.com> writes:
> On Mon, Feb 02, 2026 at 07:41:00PM -0300, Fabiano Rosas wrote:
>> Expecting every pointer member from s->parameters to be freed
>> individually is prone to leave some fields forgotten when the code is
>> eventually updated. Use a dealloc visitor to free them all at once.
>>
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>> migration/options.c | 24 +++++++++++++++++-------
>> 1 file changed, 17 insertions(+), 7 deletions(-)
>>
>> diff --git a/migration/options.c b/migration/options.c
>> index 733aae51a8..cc5a66750c 100644
>> --- a/migration/options.c
>> +++ b/migration/options.c
>> @@ -1002,11 +1002,21 @@ AnnounceParameters *migrate_announce_params(void)
>> return ≈
>> }
>>
>> -static void migrate_tls_opts_free(MigrationParameters *params)
>> +static bool migrate_params_free(MigrationParameters *params, Error **errp)
>> {
>> - qapi_free_StrOrNull(params->tls_creds);
>> - qapi_free_StrOrNull(params->tls_hostname);
>> - qapi_free_StrOrNull(params->tls_authz);
>> + Visitor *v = qapi_dealloc_visitor_new();
>> + bool ret;
>> +
>> + /*
>> + * qapi_free_MigrationParameters can't be used here because
>> + * MigrationParameters is embedded in MigrationState due to qdev
>> + * needing to access the offset of the migration properties inside
>> + * the migration object.
>> + */
>
> Ohhhhhhhh.... thanks for the comment!
>
There's some amount of rigidness caused by qdev requirements
unfortunately.
I'm not sure if I ever posted it, but I wrote some code to move the
parameters into a MigrationOptions object so we could make
MigrationState not be a TYPE_DEVICE anymore. But then we'd end up with
something like -global migration-options by default, so it kinda killed
the idea.
> Reviewed-by: Peter Xu <peterx@redhat.com>
>
>> + ret = visit_type_MigrationParameters_members(v, params, errp);
>> + visit_free(v);
>> +
>> + return ret;
>> }
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v2 8/9] migration/options: Stop freeing s->parameters members individually
2026-02-06 12:29 ` Fabiano Rosas
@ 2026-02-06 16:20 ` Peter Xu
2026-02-06 19:50 ` Fabiano Rosas
0 siblings, 1 reply; 28+ messages in thread
From: Peter Xu @ 2026-02-06 16:20 UTC (permalink / raw)
To: Fabiano Rosas; +Cc: qemu-devel, armbru, ppandit
On Fri, Feb 06, 2026 at 09:29:04AM -0300, Fabiano Rosas wrote:
> There's some amount of rigidness caused by qdev requirements
> unfortunately.
>
> I'm not sure if I ever posted it, but I wrote some code to move the
> parameters into a MigrationOptions object so we could make
> MigrationState not be a TYPE_DEVICE anymore. But then we'd end up with
> something like -global migration-options by default, so it kinda killed
> the idea.
It's not strictly about TYPE_DEVICE, but reusing of qdev properties, right?
At least the issue described by your comment was about offseting and it
sounds like so.
I just want to double check with you that I think the problem you described
will also present even after applying my other series:
https://lore.kernel.org/r/20251209162857.857593-1-peterx@redhat.com
That series only removes the TYPE_DEVICE dependency, but not qdev
properties. I think it's the qdev property trick that is relevant at least
to the offset issue you mentioned so MigrationParameters cannot be
g_new()ed.
The major use case for this qdev reuse is: (1) help scripting, so as to use
-global migration.XXX=YYY, (2) support migration in machine compat
properties. IIUC (1) isn't a major thing we ask for (again, maybe I used
the most of it.. but maybe only me; I'm not sure..), as long as anything
can keep (2) working then we can consider.
--
Peter Xu
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 8/9] migration/options: Stop freeing s->parameters members individually
2026-02-06 16:20 ` Peter Xu
@ 2026-02-06 19:50 ` Fabiano Rosas
2026-02-09 15:37 ` Peter Xu
0 siblings, 1 reply; 28+ messages in thread
From: Fabiano Rosas @ 2026-02-06 19:50 UTC (permalink / raw)
To: Peter Xu; +Cc: qemu-devel, armbru, ppandit
Peter Xu <peterx@redhat.com> writes:
> On Fri, Feb 06, 2026 at 09:29:04AM -0300, Fabiano Rosas wrote:
>> There's some amount of rigidness caused by qdev requirements
>> unfortunately.
>>
>> I'm not sure if I ever posted it, but I wrote some code to move the
>> parameters into a MigrationOptions object so we could make
>> MigrationState not be a TYPE_DEVICE anymore. But then we'd end up with
>> something like -global migration-options by default, so it kinda killed
>> the idea.
>
> It's not strictly about TYPE_DEVICE, but reusing of qdev properties, right?
> At least the issue described by your comment was about offseting and it
> sounds like so.
>
Absolutely, I was just rambling.
Of course, TYPE_DEVICE brings us weirdness such as:
(qemu) device_add migration,help
migration options:
announce-initial=<size> - (default: 50)
announce-max=<size> - (default: 550)
announce-rounds=<size> - (default: 5)
...
So it would be nice to drop this dependency.
> I just want to double check with you that I think the problem you described
> will also present even after applying my other series:
>
> https://lore.kernel.org/r/20251209162857.857593-1-peterx@redhat.com
>
> That series only removes the TYPE_DEVICE dependency, but not qdev
> properties. I think it's the qdev property trick that is relevant at least
> to the offset issue you mentioned so MigrationParameters cannot be
> g_new()ed.
>
> The major use case for this qdev reuse is: (1) help scripting, so as to use
> -global migration.XXX=YYY, (2) support migration in machine compat
> properties. IIUC (1) isn't a major thing we ask for (again, maybe I used
> the most of it.. but maybe only me; I'm not sure..), as long as anything
> can keep (2) working then we can consider.
The properties are also useful in providing defaults for the migration
options and perhaps we could make use of the .get/.set methods in a more
convenient way in the migration code, such as implementing per-option
input validation instead of checking all options always. (although I
haven't thought about the overlap with QAPI)
About -global, we should do better to separate the compat options from
the regular options. (and no, a blank line in options.c is not enough
separation!). I worry someday we'll break something in compat while
trying to change the regular options.
Another point is that even after all the recent changes to options.c, we
are still left with a list of migration_properties that is optional to
use. Which means setting defaults is also optional.
If we decide to retroactively add a default we'll suddenly have a new
option showing up in -global! Having to discover mid-way through the
implementation of a new migration feature that setting a default like
the other options do will also add property code to your option and now
you have one more source of SIGSEGV is not super fun either.
I had the intention to somehow force every option to have an equivalent
in migration_properties so that every option would have a default
explicitly set, but seeing the recent work on fixing the StrOrNull
property, I don't think it's worth it to ask that people go through the
hassle of implementing a property (when the new option cannot use the
existing types).
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 8/9] migration/options: Stop freeing s->parameters members individually
2026-02-06 19:50 ` Fabiano Rosas
@ 2026-02-09 15:37 ` Peter Xu
0 siblings, 0 replies; 28+ messages in thread
From: Peter Xu @ 2026-02-09 15:37 UTC (permalink / raw)
To: Fabiano Rosas; +Cc: qemu-devel, armbru, ppandit
On Fri, Feb 06, 2026 at 04:50:15PM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
>
> > On Fri, Feb 06, 2026 at 09:29:04AM -0300, Fabiano Rosas wrote:
> >> There's some amount of rigidness caused by qdev requirements
> >> unfortunately.
> >>
> >> I'm not sure if I ever posted it, but I wrote some code to move the
> >> parameters into a MigrationOptions object so we could make
> >> MigrationState not be a TYPE_DEVICE anymore. But then we'd end up with
> >> something like -global migration-options by default, so it kinda killed
> >> the idea.
> >
> > It's not strictly about TYPE_DEVICE, but reusing of qdev properties, right?
> > At least the issue described by your comment was about offseting and it
> > sounds like so.
> >
>
> Absolutely, I was just rambling.
>
> Of course, TYPE_DEVICE brings us weirdness such as:
>
> (qemu) device_add migration,help
> migration options:
> announce-initial=<size> - (default: 50)
> announce-max=<size> - (default: 550)
> announce-rounds=<size> - (default: 5)
> ...
>
> So it would be nice to drop this dependency.
Yep. I'll move my (below) series higher priority to respin, likely I can
drop the QOBJECT_COMPAT idea that didn't attract much attention, but
instead I can stick with exporting the qdev property helpers. It'll be
enough for us to make migration object to get rid of TYPE_DEVICE.
>
> > I just want to double check with you that I think the problem you described
> > will also present even after applying my other series:
> >
> > https://lore.kernel.org/r/20251209162857.857593-1-peterx@redhat.com
> >
> > That series only removes the TYPE_DEVICE dependency, but not qdev
> > properties. I think it's the qdev property trick that is relevant at least
> > to the offset issue you mentioned so MigrationParameters cannot be
> > g_new()ed.
> >
> > The major use case for this qdev reuse is: (1) help scripting, so as to use
> > -global migration.XXX=YYY, (2) support migration in machine compat
> > properties. IIUC (1) isn't a major thing we ask for (again, maybe I used
> > the most of it.. but maybe only me; I'm not sure..), as long as anything
> > can keep (2) working then we can consider.
>
> The properties are also useful in providing defaults for the migration
> options and perhaps we could make use of the .get/.set methods in a more
> convenient way in the migration code, such as implementing per-option
> input validation instead of checking all options always. (although I
> haven't thought about the overlap with QAPI)
Yes good point.
Said that, currently if we're still with qdev properties we can't easily
inject those checks due to the hard-coded get()/set() for qdev properties,
e.g. qdev_prop_uint8 will provide its static get()/set() hooks. We either
need to teach qdev props to take check functions, or impl our own (then
we'll need to provide the default_val and machine compat features). Maybe
it's easier to do the former, and maybe some other qdev props can leverage
too. Not an immediate concern.
>
> About -global, we should do better to separate the compat options from
> the regular options. (and no, a blank line in options.c is not enough
> separation!). I worry someday we'll break something in compat while
> trying to change the regular options.
Normally if we have something in machine compat properties, those shouldn't
be used for normal users. Those, importantly, also shouldn't be present in
QMP set parameters. That should IMHO be the line to identify a real compat
option v.s. a real parameter, because we do not expect normaly user to use
-global, only advanced, so one knows what one is doing and taking the
risks.
It actually makes sense to still be able to adjust some internal compat
behaviors if an advanced user really wants; that might be handy for
debugging in some cases to overwrite machine compat properties, even if
extremely rare.
>
> Another point is that even after all the recent changes to options.c, we
> are still left with a list of migration_properties that is optional to
> use. Which means setting defaults is also optional.
>
> If we decide to retroactively add a default we'll suddenly have a new
> option showing up in -global! Having to discover mid-way through the
> implementation of a new migration feature that setting a default like
> the other options do will also add property code to your option and now
> you have one more source of SIGSEGV is not super fun either.
>
> I had the intention to somehow force every option to have an equivalent
> in migration_properties so that every option would have a default
> explicitly set, but seeing the recent work on fixing the StrOrNull
> property, I don't think it's worth it to ask that people go through the
> hassle of implementing a property (when the new option cannot use the
> existing types).
Right. We can discuss this case by case, normally I think we should always
welcome the same parameter to be added into migration_properties[], most of
them will be simple scalars so we shouldn't worry. We can make it optional
if it needs extensive work on new types.
--
Peter Xu
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 9/9] migration: Use migrate_params_free during finalize
2026-02-02 22:40 [PATCH v2 0/9] qapi: Use visitors for migration parameters handling Fabiano Rosas
` (7 preceding siblings ...)
2026-02-02 22:41 ` [PATCH v2 8/9] migration/options: Stop freeing s->parameters members individually Fabiano Rosas
@ 2026-02-02 22:41 ` Fabiano Rosas
2026-02-05 22:21 ` Peter Xu
2026-02-05 22:05 ` [PATCH v2 0/9] qapi: Use visitors for migration parameters handling Peter Xu
9 siblings, 1 reply; 28+ messages in thread
From: Fabiano Rosas @ 2026-02-02 22:41 UTC (permalink / raw)
To: qemu-devel; +Cc: peterx, armbru, ppandit
Use the recently introduced migrate_params_free routine at
migration_instance_finalize() so that newly added pointers are already
freed by default.
Special case: The TLS options are currently the only pointers that
also have a qdev property implementation, so they will be freed by
qdev using the .release method. Update the method so that a second
invocation of qapi_free_StrOrNull doesn't assert.
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
migration/migration.c | 3 +--
migration/options.c | 7 ++++---
migration/options.h | 1 +
3 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index b103a82fc0..303626c2a9 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -3840,8 +3840,7 @@ static void migration_instance_finalize(Object *obj)
{
MigrationState *ms = MIGRATION_OBJ(obj);
- qapi_free_BitmapMigrationNodeAliasList(ms->parameters.block_bitmap_mapping);
- qapi_free_strList(ms->parameters.cpr_exec_command);
+ migrate_params_free(&ms->parameters, NULL);
qemu_mutex_destroy(&ms->error_mutex);
qemu_mutex_destroy(&ms->qemu_file_lock);
qemu_sem_destroy(&ms->wait_unplug_sem);
diff --git a/migration/options.c b/migration/options.c
index cc5a66750c..2fc86933be 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -254,8 +254,9 @@ static void set_StrOrNull(Object *obj, Visitor *v, const char *name,
static void release_StrOrNull(Object *obj, const char *name, void *opaque)
{
- const Property *prop = opaque;
- qapi_free_StrOrNull(*(StrOrNull **)object_field_prop_ptr(obj, prop));
+ StrOrNull **ptr = object_field_prop_ptr(obj, opaque);
+
+ g_clear_pointer(ptr, qapi_free_StrOrNull);
}
static void set_default_value_tls_opt(ObjectProperty *op, const Property *prop)
@@ -1002,7 +1003,7 @@ AnnounceParameters *migrate_announce_params(void)
return ≈
}
-static bool migrate_params_free(MigrationParameters *params, Error **errp)
+bool migrate_params_free(MigrationParameters *params, Error **errp)
{
Visitor *v = qapi_dealloc_visitor_new();
bool ret;
diff --git a/migration/options.h b/migration/options.h
index 0c3043f1ff..314cdba8d1 100644
--- a/migration/options.h
+++ b/migration/options.h
@@ -92,4 +92,5 @@ ZeroPageDetection migrate_zero_page_detection(void);
bool migrate_params_check(MigrationParameters *params, Error **errp);
void migrate_params_init(MigrationParameters *params);
+bool migrate_params_free(MigrationParameters *params, Error **errp);
#endif
--
2.51.0
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH v2 9/9] migration: Use migrate_params_free during finalize
2026-02-02 22:41 ` [PATCH v2 9/9] migration: Use migrate_params_free during finalize Fabiano Rosas
@ 2026-02-05 22:21 ` Peter Xu
0 siblings, 0 replies; 28+ messages in thread
From: Peter Xu @ 2026-02-05 22:21 UTC (permalink / raw)
To: Fabiano Rosas; +Cc: qemu-devel, armbru, ppandit
On Mon, Feb 02, 2026 at 07:41:01PM -0300, Fabiano Rosas wrote:
> Use the recently introduced migrate_params_free routine at
> migration_instance_finalize() so that newly added pointers are already
> freed by default.
>
> Special case: The TLS options are currently the only pointers that
> also have a qdev property implementation, so they will be freed by
> qdev using the .release method. Update the method so that a second
> invocation of qapi_free_StrOrNull doesn't assert.
>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
Reviewed-by: Peter Xu <peterx@redhat.com>
--
Peter Xu
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 0/9] qapi: Use visitors for migration parameters handling
2026-02-02 22:40 [PATCH v2 0/9] qapi: Use visitors for migration parameters handling Fabiano Rosas
` (8 preceding siblings ...)
2026-02-02 22:41 ` [PATCH v2 9/9] migration: Use migrate_params_free during finalize Fabiano Rosas
@ 2026-02-05 22:05 ` Peter Xu
2026-02-06 12:34 ` Fabiano Rosas
9 siblings, 1 reply; 28+ messages in thread
From: Peter Xu @ 2026-02-05 22:05 UTC (permalink / raw)
To: Fabiano Rosas; +Cc: qemu-devel, armbru, ppandit
On Mon, Feb 02, 2026 at 07:40:52PM -0300, Fabiano Rosas wrote:
> Hi, just a few more patches until options.c is fully using QAPI_CLONE
> instead of open-coding options handling, we're almost there.
While I'm reading your series, I found one thing that migration might be
racy against for a while, and this series should enlarge that window of
this issue: I don't think it's safe to free elements in MigrationParameters
in the main thread, even if we hold BQL. Because migration thread can
access parameters anytime without BQL... logically it can read any
parameter, then if it's not a scalar we need some luck not crashing.
IMHO we may need to move faster on the "whitelist that can be dynamically
set during migration" plan on migration parameters and capabilities.
I don't think any cap (after converted to parameters) should be on this
whitelist... for parameters, I'm trying to be conservative but I believe
the list should look like this:
CPU throttle knobs:
throttle-trigger-threshold
cpu-throttle-increment
cpu-throttle-tailslow
max-cpu-throttle
x-vcpu-dirty-limit-period
vcpu-dirty-limit
Bandwith knobs:
max-bandwidth
avail-switchover-bandwidth
downtime-limit
max-postcopy-bandwidth
COLO knob:
x-checkpoint-delay
That really should be all parameters we allow to change.. Luckily all of
them are scalars, so it's fine to keep the current migrate_params_free()
and logic to "free then update".
I wonder if we should even consider having this whitelist alongside with
your this series if we want to be extra safe, but I'll let you decide.. I'm
also OK we do it later, but maybe we don't want it to be too late either.
--
Peter Xu
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v2 0/9] qapi: Use visitors for migration parameters handling
2026-02-05 22:05 ` [PATCH v2 0/9] qapi: Use visitors for migration parameters handling Peter Xu
@ 2026-02-06 12:34 ` Fabiano Rosas
2026-02-06 17:40 ` Peter Xu
0 siblings, 1 reply; 28+ messages in thread
From: Fabiano Rosas @ 2026-02-06 12:34 UTC (permalink / raw)
To: Peter Xu; +Cc: qemu-devel, armbru, ppandit
Peter Xu <peterx@redhat.com> writes:
> On Mon, Feb 02, 2026 at 07:40:52PM -0300, Fabiano Rosas wrote:
>> Hi, just a few more patches until options.c is fully using QAPI_CLONE
>> instead of open-coding options handling, we're almost there.
>
> While I'm reading your series, I found one thing that migration might be
> racy against for a while, and this series should enlarge that window of
> this issue: I don't think it's safe to free elements in MigrationParameters
> in the main thread, even if we hold BQL. Because migration thread can
> access parameters anytime without BQL... logically it can read any
> parameter, then if it's not a scalar we need some luck not crashing.
>
Good point, I had thought of it and convinced myself it was ok
somehow. I'll take another look.
> IMHO we may need to move faster on the "whitelist that can be dynamically
> set during migration" plan on migration parameters and capabilities.
>
Or we make set-parameters always work on top of temporary
MigrationParameter objects and switch the pointer atomically. Might be a
good idea regardless.
> I don't think any cap (after converted to parameters) should be on this
> whitelist... for parameters, I'm trying to be conservative but I believe
> the list should look like this:
>
> CPU throttle knobs:
>
> throttle-trigger-threshold
> cpu-throttle-increment
> cpu-throttle-tailslow
> max-cpu-throttle
> x-vcpu-dirty-limit-period
> vcpu-dirty-limit
>
> Bandwith knobs:
>
> max-bandwidth
> avail-switchover-bandwidth
> downtime-limit
> max-postcopy-bandwidth
>
> COLO knob:
>
> x-checkpoint-delay
>
> That really should be all parameters we allow to change.. Luckily all of
> them are scalars, so it's fine to keep the current migrate_params_free()
> and logic to "free then update".
>
> I wonder if we should even consider having this whitelist alongside with
> your this series if we want to be extra safe, but I'll let you decide.. I'm
> also OK we do it later, but maybe we don't want it to be too late either.
Yeah.. I'll see what I can do. Thanks for the reviews this far!
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 0/9] qapi: Use visitors for migration parameters handling
2026-02-06 12:34 ` Fabiano Rosas
@ 2026-02-06 17:40 ` Peter Xu
2026-02-06 18:36 ` Fabiano Rosas
0 siblings, 1 reply; 28+ messages in thread
From: Peter Xu @ 2026-02-06 17:40 UTC (permalink / raw)
To: Fabiano Rosas; +Cc: qemu-devel, armbru, ppandit
On Fri, Feb 06, 2026 at 09:34:12AM -0300, Fabiano Rosas wrote:
> Or we make set-parameters always work on top of temporary
> MigrationParameter objects and switch the pointer atomically. Might be a
> good idea regardless.
Currently I recall you mentioned the parameters struct can't be an pointer,
so maybe there's some challenge there on making it atomic. But yeah, I
know you'll figure things out! :-D
--
Peter Xu
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 0/9] qapi: Use visitors for migration parameters handling
2026-02-06 17:40 ` Peter Xu
@ 2026-02-06 18:36 ` Fabiano Rosas
0 siblings, 0 replies; 28+ messages in thread
From: Fabiano Rosas @ 2026-02-06 18:36 UTC (permalink / raw)
To: Peter Xu; +Cc: qemu-devel, armbru, ppandit
Peter Xu <peterx@redhat.com> writes:
> On Fri, Feb 06, 2026 at 09:34:12AM -0300, Fabiano Rosas wrote:
>> Or we make set-parameters always work on top of temporary
>> MigrationParameter objects and switch the pointer atomically. Might be a
>> good idea regardless.
>
> Currently I recall you mentioned the parameters struct can't be an pointer,
> so maybe there's some challenge there on making it atomic. But yeah, I
> know you'll figure things out! :-D
So _that_ is why I haven't done it yet! Sometimes I doubt myself too
much =D
^ permalink raw reply [flat|nested] 28+ messages in thread