Fabiano Rosas <faro...@suse.de> writes:
> Markus Armbruster <arm...@redhat.com> writes:
>
>> Fabiano Rosas <faro...@suse.de> writes:
>>
>>> Markus Armbruster <arm...@redhat.com> writes:
>>>
>>>> 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 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 <faro...@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.
>>
>> But is NULL possible? If you just change the type from str to StrOrNull
>> and no more, it isn't.
>>
>
> Since the TLS options don't have a qdev property anymore, they also
> don't get set a default value. So s->parameters can indeed have the NULL
> value in it.
>
> I could initialize them in migrate_params_init. It's all about choosing
> where to move the complexity.
True!
> I'm leaning towards keeping it in the
> interface: query-migrate converts them to whatever it needs to output
> and set-migrate writes a normalized version into s->parameters.
There more than one way to skin this cat. I like to keep state
normalized.
State is an optional StrOrNull. Possible values:
* NULL
* QNull, i.e. non-NULL, ->type is QTYPE_QNULL
* Empty string, i.e. non-NULL, ->type is QTYPE_QSTRING, ->u.s is ""
* Non-empty string, i.e. non-NULL, -> type is QTYPE_QSTRING, ->u.s is
not "" (and cannot be NULL)
As far as I understand, we have just two cases semantically:
* Set, value is a non-empty string (empty makes no sense)
* Unset
I'd normalize the state to "either NULL, or (non-empty) string".
When writing state, we need to normalize.
When reading state, we can rely on it being normalized. Asserting it is
seems prudent, and should help readers.
[...]
>>>>> 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.
>>
>> What do we think about this external interface?
>>
>> If we think it's accidental and unused, then putting in more work to
>> keep it makes no sense.
>>
>> If we think it's deliberate and/or used, we should either keep it, or
>> replace it the orthodox way.
>>
>
> There are two external interfaces actually.
>
> -global migration.some_compat_option=on (stored in MigrationState):
>
> seems intentional and I believe we'd lose the ability to get out of some
> tricky situations if we ditched it.
>
> -global migation.some_random_option=on (stored in MigrationParameters):
>
> has become a debugging *feature*, which I personally don't use, but
> others do. And worse: we don't know if anyone uses it in production.
Accidental external interface.
> We also arbitrarily put x- in front of options for some reason. There is
> an argument to drop those because x- is scary and no one should be using
> them.
We pretty much ditched the x- convention in the QAPI schema.
docs/devel/qapi-code-gen.rst:
Names beginning with ``x-`` used to signify "experimental". This
convention has been replaced by special feature "unstable".
Goes back to
commit a3c45b3e62962f99338716b1347cfb0d427cea44
Author: Markus Armbruster <arm...@redhat.com>
Date: Thu Oct 28 12:25:12 2021 +0200
qapi: New special feature flag "unstable"
By convention, names starting with "x-" are experimental. The parts
of external interfaces so named may be withdrawn or changed
incompatibly in future releases.
The naming convention makes unstable interfaces easy to recognize.
Promoting something from experimental to stable involves a name
change. Client code needs to be updated. Occasionally bothersome.
Worse, the convention is not universally observed:
* QOM type "input-barrier" has properties "x-origin", "y-origin".
Looks accidental, but it's ABI since 4.2.
* QOM types "memory-backend-file", "memory-backend-memfd",
"memory-backend-ram", and "memory-backend-epc" have a property
"x-use-canonical-path-for-ramblock-id" that is documented to be
stable despite its name.
We could document these exceptions, but documentation helps only
humans. We want to recognize "unstable" in code, like "deprecated".
So support recognizing it the same way: introduce new special feature
flag "unstable". It will be treated specially by the QAPI generator,
like the existing feature flag "deprecated", and unlike regular
feature flags.
This commit updates documentation and prepares tests. The next commit
updates the QAPI schema. The remaining patches update the QAPI
generator and wire up -compat policy checking.
Management applications can then use query-qmp-schema and -compat to
manage or guard against use of unstable interfaces the same way as for
deprecated interfaces.
docs/devel/qapi-code-gen.txt no longer mandates the naming convention.
Using it anyway might help writers of programs that aren't
full-fledged management applications. Not using it can save us
bothersome renames. We'll see how that shakes out.
Signed-off-by: Markus Armbruster <arm...@redhat.com>
Reviewed-by: Juan Quintela <quint...@redhat.com>
Reviewed-by: John Snow <js...@redhat.com>
Message-Id: <20211028102520.747396-2-arm...@redhat.com>
The x- convention lives on in external interfaces that bypass QAPI, in
particular QOM/qdev properties.
> I think it would be good to at least separate the responsibilities so
> when the time comes we can deprecate/remove/replace the offending
> interfaces. But I won't go into that now, there's already too much
> change going on for this release.
Makes sense.
