Markus Armbruster <[email protected]> writes:
> Peter Xu <[email protected]> writes:
>
>> Hi, Markus,
>>
>> On Tue, Oct 10, 2023 at 09:18:23PM +0200, Markus Armbruster wrote:
>>
>> [...]
>>
>>> >> The point I was trying to make is this. Before the patch, we reject
>>> >> attempts to set the property value to null. Afterwards, we accept them,
>>> >> i.e. the patch loses "reject null property value". If this loss is
>>> >> undesirable, we better replace it with suitable hand-written code.
>>> >
>>> > I don't even know how to set it to NULL before.. as it can only be
>>> > accessed
>>> > via cmdline "-global" as mentioned above, which must be a string anyway.
>>> > So I assume this is not an issue.
>>>
>>> Something like
>>>
>>> {"execute": "migrate-set-parameters",
>>> "arguments": {"tls-authz": null}}
>>>
>>> Hmm, crashes in migrate_params_apply(), which is a bug. I'm getting
>>> more and more suspicious about user-facing migration code...
>>
>> Did you apply patch 1 of this series?
>
> Since we're talking about "how to set it to NULL before", I was using
> master.
>
>> https://lore.kernel.org/qemu-devel/[email protected]/
>>
>> QMP "migrate-set-parameters" does not go via migration_properties, so even
>> if we change handling of migration_properties, it shouldn't yet affect the
>> QMP interface of that.
>
> I see.
>
> I want to understand the impact of the change from 'str' to 'StrOrNull'
> on external interfaces. The first step is to know where exactly the
> type is exposed externally. *Know*, not gut-feel based on intended use.
>
> I'll have another look at the schema change, and how the types are used.
Schema changes:
1. Change MigrationParameters members @tls-creds, @tls-hostname,
@tls-authz from 'str' to 'StrOrNull'
2. Replace MigrateSetParameters by MigrationParameters.
No change, since they are identical after change 1.
To determine the patch's impact, we need to examine uses of
MigrationParameters members @tls-FOO before the patch. These are:
* Return type of query-migrate-parameters
Introspection shows the type change: the type's set of values now
includes JSON null.
Is JSON null possible? See [*] below.
* migrate_params_init()
Before the patch, we initialize to "".
Afterwards, we initialize to "" wrapped in a StrOrNull.
The initial value means "off" before and after.
* migrate_params_check()
An error check gets updated. Ignoring for now.
* migrate_params_test_apply()
Function deleted in the patch, but you wrote that's wrong. Ignoring
for now.
* migrate_params_apply()
Duplicates the three parameters from argument @parameters into the
migration object's member parameters.
Argument @parameters comes from QMP via command
migrate-set-parameters. Before the patch,
qmp_migrate_set_parameters() maps JSON null values to "". Afterwards,
it passes the values verbatim.
Parameters stored in the migration object before and after the patch:
- When initialized and never changed: char * "", and StrOrNull
QTYPE_QSTRING "".
- When set to non-empty string with migrate-set-parameters or
equivalent: that non-empty string, and QTYPE_QSTRING with that
non-empty string.
- When reset with migrate-set-parameters with value "": "", and
QTYPE_QSTRING "".
- When reset with migrate-set-parameters with value null: "", and
QTYPE_QNULL.
Note that there's now a difference between passing "" and null to
migrate-set-parameters: the former results in value QTYPE_QSTRING "",
the latter QTYPE_QNULL. Both values mean "off". I hate this. I very
much want a single C representation of "off".
* MigrationState member @parameters.
Uses:
- Properties "tls-creds", "tls-hostname", "tls-authz"
These are externally accessible with -global. The additional null
value is not accessible there: string input visitor limitation. It
could become accessible depending on how we fix the crash bugs
related to that limitation, but we can worry about that when we do
it.
Digression: why do these properties even exist? I believe we
created the "migration" (pseudo-)device just so we can use "compat
props" to apply machine- and accelerator-specific configuration
tweaks. We then added configuration for *all* configuration
parameters, not just the ones that need tweaking. The external
exposure of properties via -global is not something we wanted, it
just came with the part we wanted (compat props). Accidental
external interface. Ugh.
None of the tls-FOO are tweaked via compat props, so no worries
there.
I believe property access with qom-get and qom-set is not possible,
because the migration object is not part to the QOM tree, and
therefore is not reachable via any QOM path. Aside: feels like
abuse of QOM.
It's also not part of the device tree rooted at the main system bus,
which means it isn't visible in "info qtree". It is visible in
"info qdm", "device_add migration,help", and "-device
migration,help". Output of the latter two changes. All harmless.
I *think* that's all.
- migrate_tls(), migrate_tls_authz(), migrate_tls_creds(),
migrate_tls_hostname()
Before the patch, these return the respective migration parameter
directly. I believe the value is never NULL. Value "" is special
and means "off".
After the patch, these return the respective migration parameter
when it's a non-empty QTYPE_QSTRING, else NULL. Value NULL means
off.
Note this maps both C representations of "off" to NULL.
This changes the return value for "off" from "" to NULL.
Improvement, because it results in a more pleasant "is off" check.
- qmp_query_migrate_parameters()
The three tls_FOO get duplicated into the return value.
Looks like the two different C representations of "off" bleed into
QMP (ugh!), and [*] JSON null is possible (incompatible change).
* hmp_info_migrate_parameters()
The two different C representations of "off" are first mapped to NULL
with str_from_StrOrNull(), and then mapped to "" with a ?: operator.
Works.
Bottom line:
* Affected external interfaces:
- query-migrate-parameters: can now return either "" or null when TLS
is off. null is an incompatible change. Needs fixing.
- query-qmp-schema: shows null is now possible. Correctly reflects
the backward incompatible change. If we fix compatibility break, we
get a tolerable loss of typing precision instead.
2. Two different C representations of "off". Strong dislike. I
recommend to fix the compatibility break by switching to a single C
representation.
Thoughts?
[...]
