Re: [PATCH 1/6] migration: Fix and clean up around @tls-authz

[Dr. David Alan Gilbert]

Dan: Can you please see if this makes sense to you, it did to me
at the time.

Dave

* Dr. David Alan Gilbert (dgilbert@redhat.com) wrote:
> * Markus Armbruster (armbru@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é <berrange@redhat.com>
> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
> 
> Yes, but I'd like an Ack from Dan as well for this one
> 
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@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 / dgilbert@redhat.com / Manchester, UK
> 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK