On Wed, Oct 11, 2023 at 04:21:02PM +0200, Markus Armbruster wrote:
> > 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".
Yes.
One option to avoid such ugliness is we keep using strings for tls fields,
then the only OFF is empty string. It is not perfect either, but we need
to support empty string anyway as OFF. That's the simplest to me.
We also have the benefit of decoupling this series from the qapi string
visitor issue, which means I'll just revive v1 which still doesn't use
StrOrNull.
>
> * 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.
IIRC both of them used to be the goals: either allow compat properties for
old machine types, or specify migration parameters in cmdline for easier
debugging and tests. I use "-global" in mostly every migration test script
after it's introduced.
>
> 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.
Yes slightly abused, as the comment shows..
static const TypeInfo migration_type = {
.name = TYPE_MIGRATION,
/*
* NOTE: TYPE_MIGRATION is not really a device, as the object is
* not created using qdev_new(), it is not attached to the qdev
* device tree, and it is never realized.
*
* TODO: Make this TYPE_OBJECT once QOM provides something like
* TYPE_DEVICE's "-global" properties.
*/
.parent = TYPE_DEVICE,
It's just that current code works mostly as expected without much churns
elsewhere.. so the TODO is not really getting much attention..
>
> 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.
Maybe more than pleasant. :) Internally NULL is a must (even if empty
string), to make things like "if (tls_authz)" work.
>
> - 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.
If we want to seriously keep using StrOrNull as the interface, I think we
need to allow both NULL or "" to exist? Because the "logical goal" is to
use NULL but "" for compatibility?
I worry we spend a lot of time working on the NULL/"" but then found nobody
is using NULL anyway.
>
> - 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.
IIUC the goal is to switch to StrOrNull, but that face challenge.
>
> Thoughts?
If I'd vote (based on our current code), I'd double check tls* fields are
working properly with libvirt on empty strings, then switch all StrOrNull
in migration parameters to strings, for both QAPI and QEMU internally.
Then making sure all migrate_tls_*() helpers do proper job on converting ""
to NULL.
If we design a new interface, just remember to use NULL (hopefully) to be
clear and single way to say "OFF" starting from the 1st day.
But that's only my 2 cents.
Thanks,
--
Peter Xu
