Re: [PATCH v3 3/4] migration/qapi: Replace @MigrateSetParameters with @MigrationParameters

[Markus Armbruster <armbru@redhat.com>]

Markus Armbruster <armbru@redhat.com> writes:

> Peter Xu <peterx@redhat.com> 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/20230905162335.235619-2-peterx@redhat.com/
>>
>> 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?

[...]