On Mon, Dec 15, 2025 at 07:00:00PM -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 <[email protected]>
> ---
> migration/options.c | 25 +++++++++++++------------
> 1 file changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/migration/options.c b/migration/options.c
> index 6b60003a32..2901b37228 100644
> --- a/migration/options.c
> +++ b/migration/options.c
> @@ -1262,9 +1262,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;
> @@ -1283,24 +1283,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) {
> @@ -1357,7 +1351,6 @@ static void
> migrate_params_test_apply(MigrationParameters *params,
> }
>
> if (params->has_block_bitmap_mapping) {
> - dest->has_block_bitmap_mapping = true;
> dest->block_bitmap_mapping = params->block_bitmap_mapping;
Now "dest" came from a QAPI_CLONE, does it also need explicit free and
QAPI_CLONE() from params->block_bitmap_mapping?
I think this part looks fine when the whole set is applied, so it's only a
question of intermediate stage of this patch.
> }
>
> @@ -1532,6 +1525,14 @@ void qmp_migrate_set_parameters(MigrationParameters
> *params, Error **errp)
>
> migrate_params_test_apply(params, &tmp);
>
> + /*
> + * Mark block_bitmap_mapping as present now while we have the
> + * params structure with the user input around.
> + */
> + if (params->has_block_bitmap_mapping) {
> + migrate_get_current()->has_block_bitmap_mapping = true;
> + }
Should this be put into the if block below? Aka, when we decide to apply
the parameters?
> +
> if (migrate_params_check(&tmp, errp)) {
> migrate_params_apply(params);
> migrate_post_update_params(params, errp);
> --
> 2.51.0
>
--
Peter Xu
