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.
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>
