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