Re: [PATCH 01/21] migration: Normalize tls arguments

[Fabiano Rosas <farosas@suse.de>]

Markus Armbruster <armbru@redhat.com> writes:

> Fabiano Rosas <farosas@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 and add a
>> helper to ensure they're never actually used as QTYPE_QNULL.
>
> The type of @tls_creds, @tls-hostname, @tls-authz changes from str to
> StrOrNull in introspection query-migrate-parameters.  Loss of precision.
> Introspection is already imprecise: it shows the members optional even
> though they aren't.  We accept the loss of precision to enable
> de-duplication.
>
> This should be worked into the commit message.
>

Ack.

>> This will allow the type duplication to be removed in the next
>> patches.
>>
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  migration/migration-hmp-cmds.c |   8 +-
>>  migration/migration.c          |   2 +
>>  migration/options.c            | 149 ++++++++++++++++++++-------------
>>  migration/options.h            |   1 +
>>  migration/tls.c                |   2 +-
>>  qapi/migration.json            |   6 +-
>>  6 files changed, 99 insertions(+), 69 deletions(-)
>>
>> diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
>> index e8a563c7d8..bc8179c582 100644
>> --- a/migration/migration-hmp-cmds.c
>> +++ b/migration/migration-hmp-cmds.c
>> @@ -276,14 +276,12 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
>>          monitor_printf(mon, "%s: %u\n",
>>              MigrationParameter_str(MIGRATION_PARAMETER_MAX_CPU_THROTTLE),
>>              params->max_cpu_throttle);
>> -        assert(params->tls_creds);
>>          monitor_printf(mon, "%s: '%s'\n",
>>              MigrationParameter_str(MIGRATION_PARAMETER_TLS_CREDS),
>> -            params->tls_creds);
>> -        assert(params->tls_hostname);
>> +                       params->tls_creds ? params->tls_creds->u.s : "");
>>          monitor_printf(mon, "%s: '%s'\n",
>>              MigrationParameter_str(MIGRATION_PARAMETER_TLS_HOSTNAME),
>> -            params->tls_hostname);
>> +                       params->tls_hostname ? params->tls_hostname->u.s : "");
>>          assert(params->has_max_bandwidth);
>>          monitor_printf(mon, "%s: %" PRIu64 " bytes/second\n",
>>              MigrationParameter_str(MIGRATION_PARAMETER_MAX_BANDWIDTH),
>> @@ -319,7 +317,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->tls_authz ? params->tls_authz->u.s : "");
>>  
>>          if (params->has_block_bitmap_mapping) {
>>              const BitmapMigrationNodeAliasList *bmnal;
>
> Before, the code assumes ->tls_creds, ->tls_hostname, and ->tls_authz
> are non-null.  It assert its assumption for the first two.
>
> Afterwards, it maps null to "".  Why is that necessary?  Hmm, see below.
>

Maps NULL to "" because the intermediate type, MigrationParameters, has
been changed from str to StrOrNull. For the purposes of info
migrate_parameters and query-migrate-parameters the only valid values
are a non-empty string or an empty string.

>> diff --git a/migration/migration.c b/migration/migration.c
>> index 4697732bef..f65cb81b6d 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -4053,6 +4053,8 @@ static void migration_instance_finalize(Object *obj)
>>  {
>>      MigrationState *ms = MIGRATION_OBJ(obj);
>>  
>> +    migrate_tls_opts_free(&ms->parameters);
>
> Is this a bug fix?
>

My new version is a little different, but I can make it a separate patch
if that happens to be the case.

> As far as I can tell, the object gets destroyed only on QEMU shutdown.
> Freeing resources then is unnecessary, except it may help leak detection
> tools.
>

From a maintenance perspective I consider any leak as a bug because I
don't have control over what kinds of leak detection tools are ran on
the code. To avoid spending time looking at such bug reports I have ASAN
automated in my testing and I fix early any leaks that it founds.

>> +
>>      qemu_mutex_destroy(&ms->error_mutex);
>>      qemu_mutex_destroy(&ms->qemu_file_lock);
>>      qemu_sem_destroy(&ms->wait_unplug_sem);
>> diff --git a/migration/options.c b/migration/options.c
>> index 162c72cda4..45a95dc6da 100644
>> --- a/migration/options.c
>> +++ b/migration/options.c
>> @@ -162,9 +162,11 @@ const Property migration_properties[] = {
>>      DEFINE_PROP_SIZE("announce-step", MigrationState,
>>                        parameters.announce_step,
>>                        DEFAULT_MIGRATE_ANNOUNCE_STEP),
>> -    DEFINE_PROP_STRING("tls-creds", MigrationState, parameters.tls_creds),
>> -    DEFINE_PROP_STRING("tls-hostname", MigrationState, parameters.tls_hostname),
>> -    DEFINE_PROP_STRING("tls-authz", MigrationState, parameters.tls_authz),
>> +    /*
>> +     * tls-creds, tls-hostname and tls-authz are of type StrOrNull,
>> +     * which can't be easily handled (if at all) by qdev. So these
>> +     * will not be exposed as global migration options (-global).
>> +     */
>
> This is a compatibility break.
>
> The orthodox way to break it is deprecate, let the grace period expire,
> break.  Record in docs/about/deprecated.rst at the beginning, move the
> record to docs/about/removed-features.rst at the end.
>
> An argument could be made that the interface in question is
> accidental[*], not actually used by anything, and therefore breaking it
> without a grace period is fine.  But even then we should record the
> breakage in docs/about/removed-features.rst.
>

Ok. Alternatively I could try a little harder to keep these
options. I'll see what I can do.

> Aside: the interface in question is a hack (making the migration object
> a device) piled onto a hack (the way compat properties work, and how
> they spill into -global).  Past sins catching up with us...
>

Hm, I've been experimenting with adding new objects (still TYPE_DEVICE)
to hold the compatibility options and parameters only. Not the entire
migration state. The MigrationState object would then be converted into
a regular struct.

MigrationState => internal type holding migration state (a better name
could be used)
MigrationOptionsState => -global migration-options.foo=on
MigrationCompatState => -global migration foo=on

But it's getting a little tricky due to s->parameters *also* containing
compat options, namely zero-page-detection.

>>      DEFINE_PROP_UINT64("x-vcpu-dirty-limit-period", MigrationState,
>>                         parameters.x_vcpu_dirty_limit_period,
>>                         DEFAULT_MIGRATE_VCPU_DIRTY_LIMIT_PERIOD),
>> @@ -379,13 +381,6 @@ bool migrate_rdma(void)
>>      return s->rdma_migration;
>>  }
>>  
>> -bool migrate_tls(void)
>> -{
>> -    MigrationState *s = migrate_get_current();
>> -
>> -    return s->parameters.tls_creds && *s->parameters.tls_creds;
>> -}
>> -
>>  typedef enum WriteTrackingSupport {
>>      WT_SUPPORT_UNKNOWN = 0,
>>      WT_SUPPORT_ABSENT,
>> @@ -834,21 +829,44 @@ const char *migrate_tls_authz(void)
>>  {
>>      MigrationState *s = migrate_get_current();
>>  
>> -    return s->parameters.tls_authz;
>> +    if (s->parameters.tls_authz &&
>> +        s->parameters.tls_authz->type == QTYPE_QSTRING &&
>> +        *s->parameters.tls_authz->u.s) {
>> +        return s->parameters.tls_authz->u.s;
>> +    }
>> +
>> +    return NULL;
>>  }
>>  
>>  const char *migrate_tls_creds(void)
>>  {
>>      MigrationState *s = migrate_get_current();
>>  
>> -    return s->parameters.tls_creds;
>> +    if (s->parameters.tls_creds &&
>> +        s->parameters.tls_creds->type == QTYPE_QSTRING &&
>> +        *s->parameters.tls_creds->u.s) {
>> +        return s->parameters.tls_creds->u.s;
>> +    }
>> +
>> +    return NULL;
>>  }
>>  
>>  const char *migrate_tls_hostname(void)
>>  {
>>      MigrationState *s = migrate_get_current();
>>  
>> -    return s->parameters.tls_hostname;
>> +    if (s->parameters.tls_hostname &&
>> +        s->parameters.tls_hostname->type == QTYPE_QSTRING &&
>> +        *s->parameters.tls_hostname->u.s) {
>> +        return s->parameters.tls_hostname->u.s;
>> +    }
>> +
>> +    return NULL;
>> +}
>
> Again, the code changes to cope with null.  Why is that necessary?
> Again, see below.
>

This is actually a bit roundabout indeed. In my new version I do:

- migrate-set-parameters: always write either NULL or a non-empty
  QTYPE_QSTRING to s->parameters.
    
- query-migrate-parameters: always return a (possibly empty)
  QTYPE_QSTRING.

With this, internal representation is: NULL for not present, non-empty
string for present.

>> +
>> +bool migrate_tls(void)
>> +{
>> +    return !!migrate_tls_creds();
>>  }
>>  
>>  uint64_t migrate_vcpu_dirty_limit_period(void)
>> @@ -888,6 +906,36 @@ AnnounceParameters *migrate_announce_params(void)
>>      return &ap;
>>  }
>>  
>> +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);
>> +}
>> +
>> +/* needs BQL if dst is part of s->parameters */
>> +static void tls_option_set_str(StrOrNull **dstp, StrOrNull *src)
>> +{
>> +    StrOrNull *dst = *dstp;
>> +
>> +    assert(!dst);
>> +
>> +    dst = *dstp = g_new0(StrOrNull, 1);
>> +    dst->type = QTYPE_QSTRING;
>> +
>> +    if (!src) {
>> +        dst->u.s = g_strdup("");
>> +        return;
>> +    }
>> +
>> +    if (src->type == QTYPE_QSTRING) {
>> +        dst->u.s = g_strdup(src->u.s);
>> +    } else {
>> +        assert(src->type == QTYPE_QNULL);
>> +        dst->u.s = g_strdup("");
>> +    }
>> +}
>
> Postcondition: dstp points to a StrOrNull containing a str,
> i.e. QTYPE_QSTRING.  Makes sense.
>
> I'd prefer something like
>
>        StrOrNull *dst = g_new0(StrOrNull, 1);
>
>        ... fill in members ...
>
>        assert(!*dstp);
>        *dstp = dst;
>

This code is also reworked a bit on the next version, I'll see whether
the comment still applies.

>> +
>>  MigrationParameters *qmp_query_migrate_parameters(Error **errp)
>>  {
>>      MigrationParameters *params;
>> @@ -903,10 +951,11 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
>>      params->cpu_throttle_increment = s->parameters.cpu_throttle_increment;
>>      params->has_cpu_throttle_tailslow = true;
>>      params->cpu_throttle_tailslow = s->parameters.cpu_throttle_tailslow;
>> -    params->tls_creds = g_strdup(s->parameters.tls_creds);
>> -    params->tls_hostname = g_strdup(s->parameters.tls_hostname);
>> -    params->tls_authz = g_strdup(s->parameters.tls_authz ?
>> -                                 s->parameters.tls_authz : "");
>> +
>> +    tls_option_set_str(&params->tls_creds, s->parameters.tls_creds);
>> +    tls_option_set_str(&params->tls_hostname, s->parameters.tls_hostname);
>> +    tls_option_set_str(&params->tls_authz, s->parameters.tls_authz);
>> +
>>      params->has_max_bandwidth = true;
>>      params->max_bandwidth = s->parameters.max_bandwidth;
>>      params->has_avail_switchover_bandwidth = true;
>> @@ -963,9 +1012,6 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
>>  
>>  void migrate_params_init(MigrationParameters *params)
>>  {
>> -    params->tls_hostname = g_strdup("");
>> -    params->tls_creds = g_strdup("");
>> -
>
> Is this the reason why the code now needs to deal with null?
>
> I'm not objecting, just pointing out that the commit message didn't
> prepare me for such a change.
>

I'll document it.

>>      /* Set has_* up only for parameter checks */
>>      params->has_throttle_trigger_threshold = true;
>>      params->has_cpu_throttle_initial = true;
>
> I'm stopping here to ask: how exactly does the patch change quasi-global
> state, namely current_migration->parameters->tls_*?
>

It makes sure the current_migration->parameters->tls_* options are
always QTYPE_QSTRING and either a non-empty or an empty string.

The next version of the patch will instead use non-empty QTYPE_QSTRING
or NULL, which is cleaner from a C perspective.

Both versions ensure the query-* and set-* commands continue to expose
the same values. Query only shows non-empty or empty string and Set
accepts all values of a StrOrNull type.

> [...]
>
>
> [*] We have oh so many accidental external interfaces.