Re: [PATCH 5/5] migration/options: Use QAPI_MERGE in migrate_params_test_apply

[Fabiano Rosas <farosas@suse.de>]

Prasad Pandit <ppandit@redhat.com> writes:

> Hi,
>
> On Thu, 22 Jan 2026 at 03:41, Fabiano Rosas <farosas@suse.de> wrote:
>> See if the following looks better to you. It's not one single function,
>> but I folded what I could while keeping the distinct function names.
>>
>> static void migrate_params_apply(MigrationParameters *new, Error **errp)
>> {
>>     MigrationState *s = migrate_get_current();
>>     MigrationParameters *cur = &s->parameters;
>>     MigrationParameters *tmp = QAPI_CLONE(MigrationParameters, cur);
>>
>>     QAPI_MERGE(MigrationParameters, tmp, new);
>>
>>     if (!migrate_params_check(tmp, errp)) {
>>         return;
>>     }
>>
>>     migrate_ptr_opts_free(cur);
>>
>>     /* mark all present, so they're all copied */
>>     migrate_mark_all_params_present(params);
>>     QAPI_CLONE_MEMBERS(MigrationParameters, cur, params);
>>
>>     migrate_post_update_params(new, errp);
>> }
>>
>> void qmp_migrate_set_parameters(MigrationParameters *new, Error **errp)
>> {
>>     /*
>>      * Convert QTYPE_QNULL and NULL to the empty string (""). Even
>>      * though NULL is cleaner to deal with in C code, that would force
>>      * query-migrate-parameters to convert it once more to the empty
>>      * string, so avoid that. The migrate_tls_*() helpers that expose
>>      * the options to the rest of the migration code already use
>>      * return NULL when the empty string is found.
>>      */
>>     tls_opt_to_str(new->tls_creds);
>>     tls_opt_to_str(new->tls_hostname);
>>     tls_opt_to_str(new->tls_authz);
>>
>>     migrate_params_apply(new, errp);
>> }
>>
>
> * I'm thinking as:
> ===
> v0:
>
> void qmp_migrate_set_parameters(MigrationParameters *new, Error **errp)
> {
>     /*
>      * Convert QTYPE_QNULL and NULL to the empty string (""). Even
>      * though NULL is cleaner to deal with in C code, that would force
>      * query-migrate-parameters to convert it once more to the empty
>      * string, so avoid that. The migrate_tls_*() helpers that expose
>      * the options to the rest of the migration code already use
>      * return NULL when the empty string is found.
>      */
>     tls_opt_to_str(new->tls_creds);
>     tls_opt_to_str(new->tls_hostname);
>     tls_opt_to_str(new->tls_authz);
>
>     MigrationState *s = migrate_get_current();
>     MigrationParameters *cur = &s->parameters;
>     MigrationParameters *tmp = QAPI_CLONE(MigrationParameters, cur);
>
>     QAPI_MERGE(MigrationParameters, tmp, new);
>
>     if (!migrate_params_check(tmp, errp)) {
>         return;
>     }
>     migrate_ptr_opts_free(cur);
>
>     /* mark all present, so they're all copied */
>     migrate_mark_all_params_present(tpm);
>     QAPI_CLONE_MEMBERS(MigrationParameters, cur, tmp);
>
>     migrate_post_update_params(cur, errp);
> }
> ===
> v1:
>
> void qmp_migrate_set_parameters(MigrationParameters *new, Error **errp)
> {
>     /*
>      * Convert QTYPE_QNULL and NULL to the empty string (""). Even
>      * though NULL is cleaner to deal with in C code, that would force
>      * query-migrate-parameters to convert it once more to the empty
>      * string, so avoid that. The migrate_tls_*() helpers that expose
>      * the options to the rest of the migration code already use
>      * return NULL when the empty string is found.
>      */
>     tls_opt_to_str(new->tls_creds);
>     tls_opt_to_str(new->tls_hostname);
>     tls_opt_to_str(new->tls_authz);
>
>     MigrationState *s = migrate_get_current();
>     MigrationParameters *cur = &s->parameters;
>     MigrationParameters *tmp = QAPI_CLONE(MigrationParameters, cur);      ...[1]
>
>     QAPI_MERGE(MigrationParameters, tmp, new);
>                  ...[2]
>
>     if (!migrate_params_check(tmp, errp)) {
>         return;
>     }
>     migrate_ptr_opts_free(cur);
>
>     /* mark all present, so they're all copied */
>     migrate_mark_all_params_present(tpm);
>     QAPI_CLONE_MEMBERS(MigrationParameters, cur, tmp);
>        ...[3]
>
>     /* update MigrationState after parameters change */
>                      ...[4]
>     if (cur->has_max_bandwidth) {
>         if (s->to_dst_file && !migration_in_postcopy()) {
>             migration_rate_set(cur->max_bandwidth);
>         }
>     }
>     if (cur->has_x_checkpoint_delay) {
>         colo_checkpoint_delay_set();
>     }
>     if (cur->has_xbzrle_cache_size) {
>         xbzrle_cache_resize(cur->xbzrle_cache_size, errp);
>     }
>     if (cur->has_max_postcopy_bandwidth) {
>         if (s->to_dst_file && migration_in_postcopy()) {
>             migration_rate_set(cur->max_postcopy_bandwidth);
>         }
>     }
> }
> ===
>
> * I'd go with v1 above.
> * Reasons for folding migrate_params_apply() and migrate_post_update_params():
>    - Both static functions are called _only_ from
> qmp_migrate_set_parameters(). They are not reusable from elsewhere.
>    - Earlier migrate_params_apply() was quite long due to  if...else
> conditionals, so it made sense to break one long function into 2-3
> smaller ones.
>    - Now with QAPI_CLONE/MERGE changes qmp_migrate_set_parameters() is
> concise and easy to follow.
>    - Reader can see all steps (1,2,3,4 above) together in one function.
>
> * Just sharing my thoughts. You pick what you think is best.
>

Ok, thanks, let's see in what mood I'm in when reposting =)

> Thank you.
> ---
>   - Prasad