Prasad Pandit <[email protected]> writes: > 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. >
Ok, thanks, let's see in what mood I'm in when reposting =) > Thank you. > --- > - Prasad
