Re: [PATCH v2 03/24] migration: Normalize tls arguments

[Fabiano Rosas <farosas@suse.de>]

Bin Guo <guobin@linux.alibaba.com> writes:

> Fabiano Rosas wrote on Mon, 30 Jun 2025 16:58:52 -0300:
>
>> The migration parameters tls_creds, tls_authz and tls_hostname
>> currently have a non-uniform handling. When used as arguments to
>> migrate-set-parameters, their type is StrOrNull and when used as
>> return value from query-migrate-parameters their type is a plain
>> string.
>> 
>> Not only having to convert between the types is cumbersome, but it
>> also creates the issue of requiring two different QAPI types to be
>> used, one for each command. MigrateSetParameters is used for
>> migrate-set-parameters with the TLS arguments as StrOrNull while
>> MigrationParameters is used for query-migrate-parameters with the TLS
>> arguments as str.
>> 
>> Since StrOrNull could be considered a superset of str, change the type
>> of the TLS arguments in MigrationParameters to StrOrNull. Also ensure
>> that QTYPE_QNULL is never used.
>> 
>> 1) migrate-set-parameters will always write QTYPE_QSTRING to
>>   s->parameters, either an empty or non-empty string.
>> 
>> 2) query-migrate-parameters will always return a QTYPE_QSTRING, either
>>   empty or non-empty.
>> 
>> 3) the migrate_tls_* helpers will always return a non-empty string or
>>   NULL, for the internal migration code's consumption.
>> 
>> Points (1) and (2) above help simplify the parameters validation and
>> the query command handling because s->parameters is already kept in
>> the format that query-migrate-parameters (and info migrate_paramters)
>> expect. Point (3) is so people don't need to care about StrOrNull in
>> migration code.
>> 
>> This will allow the type duplication to be removed in the next
>> patches.
>> 
>> Note that the type of @tls_creds, @tls-hostname, @tls-authz changes
>> from str to StrOrNull in introspection of the query-migrate-parameters
>> command. We accept this imprecision to enable de-duplication.
>> 
>> There's no need to free the TLS options in
>> migration_instance_finalize() because they're freed by the qdev
>> properties .release method.
>> 
>> Temporary in this patch:
>> migrate_params_test_apply() copies s->parameters into a temporary
>> structure, so it's necessary to drop the references to the TLS options
>> if they were not set by the user to avoid double-free. This is fixed
>> in the next patches.
>> 
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  migration/migration-hmp-cmds.c |   7 +-
>>  migration/options.c            | 156 ++++++++++++++++++++-------------
>>  migration/options.h            |   1 +
>>  migration/tls.c                |   2 +-
>>  qapi/migration.json            |   6 +-
>>  5 files changed, 106 insertions(+), 66 deletions(-)
>> 
>> +void migrate_tls_opts_free(MigrationParameters *params)
>> +{
>> +    qapi_free_StrOrNull(params->tls_creds);
>> +    qapi_free_StrOrNull(params->tls_hostname);
>> +    qapi_free_StrOrNull(params->tls_authz);
>> +}
>> +
>> +/* either non-empty or empty string */
>> +static void tls_opt_to_str(StrOrNull **tls_opt)
>> +{
>> +    StrOrNull *opt = *tls_opt;
>> +
>> +    if (!opt) {
>> +        return;
>> +    }
>> +
>> +    switch (opt->type) {
>> +    case QTYPE_QSTRING:
>> +        return;
>> +    case QTYPE_QNULL:
>> +        qobject_unref(opt->u.n);
>> +        break;
>> +    default:
>> +        g_assert_not_reached();
>> +    }
>> +
>> +    opt->type = QTYPE_QSTRING;
>> +    opt->u.s = g_strdup("");
>> +    *tls_opt = opt;
>> +}
>> +
>
> Can these two functions be changed to be general, not just for tls?
> IMHO: 
> void migrate_strornull_opts_free(MigrationParameters *params)
> boid strornull_to_str(StrOrNull **the_opt)
>
> In this way, when members of type StrOrNull are added later,
> the function can be reused.
>

Ah, that's a clever observation, but we should probably refrain from
adding more StrOrNull options. The original intent behind the creation
of this type was already to workaround issues with the TLS options. I'd
rather leave it like this.

> --
> Bin Guo