Re: [PATCH v3 05/51] migration: Normalize tls arguments

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@redhat.com> writes:

> On Mon, Dec 15, 2025 at 06:59:51PM -0300, Fabiano Rosas wrote:
>> 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.
>> 
>> Acked-by: Markus Armbruster <armbru@redhat.com>
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>
> [...]
>
>> @@ -403,6 +403,9 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
>>          monitor_printf(mon, "%s: %" PRIu64 " bytes\n",
>>              MigrationParameter_str(MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE),
>>              params->xbzrle_cache_size);
>> +        monitor_printf(mon, "%s: %" PRIu64 "\n",
>> +            MigrationParameter_str(MIGRATION_PARAMETER_MAX_POSTCOPY_BANDWIDTH),
>> +            params->max_postcopy_bandwidth);
>>  
>>          if (params->has_block_bitmap_mapping) {
>>              const BitmapMigrationNodeAliasList *bmnal;
>
> This chunk seems to be introduced by accident and removed in patch 18..
>

Yes, rebase mistake. Thanks.