Dan: Can you please see if this makes sense to you, it did to me
at the time.
Dave
* Dr. David Alan Gilbert (dgilb...@redhat.com) wrote:
> * 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
>
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
