Re: [PATCH v2 14/24] migration: Use visitors in migrate_params_test_apply

[Bin Guo <guobin@linux.alibaba.com>]

Fabiano Rosas wrote on Mon, 30 Jun 2025 16:59:03 -0300:

> 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.
> 
> Change the name of the function to a more direct reference of what it
> does: merging the user params with the temporary copy.
> 
> 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 migrate_params_apply() can copy all
> of them.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  migration/options.c | 157 +++++++++++++++-----------------------------
>  1 file changed, 54 insertions(+), 103 deletions(-)
> 
> diff --git a/migration/options.c b/migration/options.c
> index 6619b5f21a..695bec5b8f 100644
> --- a/migration/options.c
> +++ b/migration/options.c
> 
> -static void migrate_params_test_apply(MigrationParameters *params,
> -                                      MigrationParameters *dest)
> +static bool migrate_params_merge(MigrationParameters *dst,
> +
> +    ...
> +    /* read in from src */
> +    v = qobject_output_visitor_new(&ret_out);
> +    ok = visit_type_MigrationParameters(v, NULL, &src, errp);
> +    if (!ok) {
> +        goto out;
>      }
> +    visit_complete(v, &ret_out);
> +    visit_free(v);
>  
> -    if (params->has_max_bandwidth) {
> -        dest->max_bandwidth = params->max_bandwidth;
> +    /*
> +     * 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(ret_out);
> +    ok = visit_start_struct(v, NULL, NULL, 0, errp);
> +    if (!ok) {
> +        goto out;
>      }
> -
> -    if (params->has_avail_switchover_bandwidth) {
> -        dest->avail_switchover_bandwidth = params->avail_switchover_bandwidth;
> +    ok = visit_type_MigrationParameters_members(v, dst, errp);
> +    if (!ok) {
> +        goto out;
>      }
> -
> -    if (params->has_downtime_limit) {
> -        dest->downtime_limit = params->downtime_limit;
> +    ok = visit_check_struct(v, errp);
> +    visit_end_struct(v, NULL);
> +    if (!ok) {
> +        goto out;
>      }
>  
> -    if (params->has_x_checkpoint_delay) {
> -        dest->x_checkpoint_delay = params->x_checkpoint_delay;
> -    }
> +out:
> +    visit_free(v);
> +    qobject_unref(ret_out);
> +    return ok;
> }

If visit_start_struct is executed successfully, then visit_end_struct
should be executed. IMHO:

    v = qobject_input_visitor_new(ret_out);
    ok = visit_start_struct(v, NULL, NULL, 0, errp);
    if (!ok) {
        goto out;
    }

    ok = visit_type_MigrationParameters_members(v, dst, errp);
    if (!ok) {
        goto out_end;
    }

    ok = visit_check_struct(v, errp);

out_end:
    visit_end_struct(v, NULL);

out:
    visit_free(v);
    qobject_unref(ret_out);

    return ok;
}


>  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
> @@ -1367,7 +1316,9 @@ 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);
> +    if (!migrate_params_merge(tmp, params, errp)) {
> +        return;
> +    }
>  
>      /*
>       * Mark block_bitmap_mapping as present now while we have the
> @@ -1377,10 +1328,10 @@ void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
>          migrate_get_current()->has_block_bitmap_mapping = true;
>      }

s->has_block_bitmap_mapping = true;

>  
> -    if (migrate_params_check(&tmp, errp)) {
> -        migrate_params_apply(&tmp);
> +    if (migrate_params_check(tmp, errp)) {
> +        /* mark all present, so they're all copied */
> +        migrate_mark_all_params_present(tmp);
> +        migrate_params_apply(tmp);
>          migrate_post_update_params(params, errp);
>      }
> -
> -    migrate_tls_opts_free(&tmp);
>  }

--
Bin Guo