Hi,
On Thu, 22 Jan 2026 at 03:41, Fabiano Rosas <[email protected]> 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.
Thank you.
---
- Prasad
