* Markus Armbruster (arm...@redhat.com) wrote:
> Commit d2f1d29b95 "migration: add support for a "tls-authz" migration
> parameter" added MigrationParameters member @tls-authz. Whereas the
> other members aren't really optional (see commit 1bda8b3c695), this
> one is genuinely optional: migration_instance_init() leaves it absent,
> and migration_tls_channel_process_incoming() passes it to
> qcrypto_tls_session_new(), which checks for null.
>
> Commit d2f1d29b95 has a number of issues, though:
>
> * When qmp_query_migrate_parameters() copies migration parameters into
> its reply, it ignores has_tls_authz, and assumes true instead. When
> it is false,
>
> - HMP info migrate_parameters prints the null pointer (crash bug on
> some systems), and
>
> - QMP query-migrate-parameters replies "tls-authz": "" (because the
> QObject output visitor silently maps null pointer to "", which it
> really shouldn't).
>
> The HMP defect was noticed and fixed in commit 7cd75cbdb8
> 'migration: use "" instead of (null) for tls-authz'. Unfortunately,
> the fix papered over the real bug: it made
> qmp_query_migrate_parameters() map null tls_authz to "". It also
> dropped the check for has_tls_authz from
> hmp_info_migrate_parameters().
>
> Revert, and fix qmp_query_migrate_parameters() not to screw up
> has_tls_authz. No change to HMP. QMP now has "tls-authz" in the
> reply only when it's actually present in
> migrate_get_current()->parameters. If we prefer to remain
> bug-compatible, we should make tls_authz non-optional there.
>
> * migrate_params_test_apply() neglects to apply tls_authz. Currently
> harmless, because migrate_params_check() doesn't care. Fix it
> anyway.
>
> * qmp_migrate_set_parameters() crashes:
>
> {"execute": "migrate-set-parameters", "arguments": {"tls-authz": null}}
>
> Add the necessary rewrite of null to "". For background
> information, see commit 01fa559826 "migration: Use JSON null instead
> of "" to reset parameter to default".
>
> Fixes: d2f1d29b95aa45d13262b39153ff501ed6b1ac95
> Cc: Daniel P. Berrangé <berra...@redhat.com>
> Signed-off-by: Markus Armbruster <arm...@redhat.com>
Yes, but I'd like an Ack from Dan as well for this one
Reviewed-by: Dr. David Alan Gilbert <dgilb...@redhat.com>
> ---
> qapi/migration.json | 2 +-
> migration/migration.c | 17 ++++++++++++++---
> monitor/hmp-cmds.c | 2 +-
> 3 files changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 3c75820527..688e8da749 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -928,7 +928,7 @@
> ##
> # @MigrationParameters:
> #
> -# The optional members aren't actually optional.
> +# The optional members aren't actually optional, except for @tls-authz.
> #
> # @announce-initial: Initial delay (in milliseconds) before sending the
> # first announce (Since 4.0)
> diff --git a/migration/migration.c b/migration/migration.c
> index 3263aa55a9..cad56fbf8c 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -855,9 +855,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error
> **errp)
> params->tls_creds = g_strdup(s->parameters.tls_creds);
> params->has_tls_hostname = true;
> params->tls_hostname = g_strdup(s->parameters.tls_hostname);
> - params->has_tls_authz = true;
> - params->tls_authz = g_strdup(s->parameters.tls_authz ?
> - s->parameters.tls_authz : "");
> + params->has_tls_authz = s->parameters.has_tls_authz;
> + params->tls_authz = g_strdup(s->parameters.tls_authz);
> params->has_max_bandwidth = true;
> params->max_bandwidth = s->parameters.max_bandwidth;
> params->has_downtime_limit = true;
> @@ -1433,6 +1432,11 @@ static void
> migrate_params_test_apply(MigrateSetParameters *params,
> dest->tls_hostname = params->tls_hostname->u.s;
> }
>
> + if (params->has_tls_authz) {
> + assert(params->tls_authz->type == QTYPE_QSTRING);
> + dest->tls_authz = params->tls_authz->u.s;
> + }
> +
> if (params->has_max_bandwidth) {
> dest->max_bandwidth = params->max_bandwidth;
> }
> @@ -1622,6 +1626,13 @@ void qmp_migrate_set_parameters(MigrateSetParameters
> *params, Error **errp)
> params->tls_hostname->type = QTYPE_QSTRING;
> params->tls_hostname->u.s = strdup("");
> }
> + /* TODO Rewrite "" to null instead */
> + if (params->has_tls_authz
> + && params->tls_authz->type == QTYPE_QNULL) {
> + qobject_unref(params->tls_authz->u.n);
> + params->tls_authz->type = QTYPE_QSTRING;
> + params->tls_authz->u.s = strdup("");
> + }
>
> migrate_params_test_apply(params, &tmp);
>
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index a6a6684df1..492789248f 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -476,7 +476,7 @@ void hmp_info_migrate_parameters(Monitor *mon, const
> QDict *qdict)
> params->max_postcopy_bandwidth);
> monitor_printf(mon, "%s: '%s'\n",
> MigrationParameter_str(MIGRATION_PARAMETER_TLS_AUTHZ),
> - params->tls_authz);
> + params->has_tls_authz ? params->tls_authz : "");
>
> if (params->has_block_bitmap_mapping) {
> const BitmapMigrationNodeAliasList *bmnal;
> --
> 2.26.2
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
