Markus Armbruster <arm...@redhat.com> writes:
> Fabiano Rosas <faro...@suse.de> writes:
>
>> 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 <faro...@suse.de>
>
> [...]
>
>> diff --git a/migration/options.c b/migration/options.c
>> index 384ef9e421..f7bbdba5fc 100644
>> --- a/migration/options.c
>> +++ b/migration/options.c
>
> [...]
>
>> @@ -935,6 +951,37 @@ AnnounceParameters *migrate_announce_params(void)
>> return ≈
>> }
>>
>> +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 */
>
> This isn't true, because ...
>
>> +static void tls_opt_to_str(StrOrNull **tls_opt)
>> +{
>> + StrOrNull *opt = *tls_opt;
>> +
>> + if (!opt) {
>> + return;
>
> ... it can also be null.
>
Hmm, I'll have to double check, but with the StrOrNull property being
initialized, NULL should not be possible. This looks like a mistake.
> Maybe
>
> /* Normalize QTYPE_QNULL to QTYPE_QSTRING "" */
>
>> + }
>> +
>> + 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;
>> +}
>
> I'd prefer something like
>
> if (!opt || opt->type == QTYPE_QSTRING) {
> return;
> }
> qobject_unref(opt->u.n);
> opt->type = QTYPE_QSTRING;
> opt->u.s = g_strdup("");
> *tls_opt = opt;
>
> But this is clearly a matter of taste.
>
>> +
>> MigrationParameters *qmp_query_migrate_parameters(Error **errp)
>> {
>> MigrationParameters *params;
>
> [...]
>
>> @@ -1251,18 +1294,24 @@ static void
>> migrate_params_test_apply(MigrateSetParameters *params,
>> }
>>
>> if (params->tls_creds) {
>> - assert(params->tls_creds->type == QTYPE_QSTRING);
>> - dest->tls_creds = params->tls_creds->u.s;
>> + dest->tls_creds = QAPI_CLONE(StrOrNull, params->tls_creds);
>> + } else {
>> + /* drop the reference, it's owned by s->parameters */
>> + dest->tls_creds = NULL;
>
> Suggest "clear the reference" to avoid associations with reference
> counting.
>
>> }
>>
>> if (params->tls_hostname) {
>> - assert(params->tls_hostname->type == QTYPE_QSTRING);
>> - dest->tls_hostname = params->tls_hostname->u.s;
>> + dest->tls_hostname = QAPI_CLONE(StrOrNull, params->tls_hostname);
>> + } else {
>> + /* drop the reference, it's owned by s->parameters */
>> + dest->tls_hostname = NULL;
>> }
>>
>> if (params->tls_authz) {
>> - assert(params->tls_authz->type == QTYPE_QSTRING);
>> - dest->tls_authz = params->tls_authz->u.s;
>> + dest->tls_authz = QAPI_CLONE(StrOrNull, params->tls_authz);
>> + } else {
>> + /* drop the reference, it's owned by s->parameters */
>> + dest->tls_authz = NULL;
>> }
>>
>> if (params->has_max_bandwidth) {
>
> [...]
>
>> diff --git a/qapi/migration.json b/qapi/migration.json
>> index 4963f6ca12..97bb022c45 100644
>> --- a/qapi/migration.json
>> +++ b/qapi/migration.json
>> @@ -1293,9 +1293,9 @@
>> '*cpu-throttle-initial': 'uint8',
>> '*cpu-throttle-increment': 'uint8',
>> '*cpu-throttle-tailslow': 'bool',
>> - '*tls-creds': 'str',
>> - '*tls-hostname': 'str',
>> - '*tls-authz': 'str',
>> + '*tls-creds': 'StrOrNull',
>> + '*tls-hostname': 'StrOrNull',
>> + '*tls-authz': 'StrOrNull',
>> '*max-bandwidth': 'size',
>> '*avail-switchover-bandwidth': 'size',
>> '*downtime-limit': 'uint64',
>
> QAPI schema
> Acked-by: Markus Armbruster <arm...@redhat.com>
