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

[Daniel P . Berrangé]

On Fri, Aug 25, 2023 at 01:15:16PM -0400, Peter Xu wrote:
> Quotting from Markus in his replies:
> 
>   migrate-set-parameters sets migration parameters, and
>   query-migrate-parameters gets them.  Unsurprisingly, the former's
>   argument type MigrateSetParameters is quite close to the latter's
>   return type MigrationParameters.  The differences are subtle:
> 
>   1. Since migrate-set-parameters supports setting selected parameters,
>      its arguments must all be optional (so you can omit the ones you
>      don't want to change).  query-migrate-parameters results are also
>      all optional, but almost all of them are in fact always present.
> 
>   2. For parameters @tls_creds, @tls_hostname, @tls_authz,
>      migrate-set-parameters interprets special value "" as "reset to
>      default".  Works, because "" is semantically invalid.  Not a
>      general solution, because a semantically invalid value need not
>      exist.  Markus added a general solution in commit 01fa559826
>      ("migration: Use JSON null instead of "" to reset parameter to
>      default").  This involved changing the type from 'str' to
>      'StrOrNull'.
> 
>   3. When parameter @block-bitmap-mapping has not been set,
>      query-migrate-parameters does not return it (absent optional
>      member).  Clean (but undocumented).  When parameters @tls_creds,
>      @tls_hostname, @tls_authz have not been set, it returns the
>      semantically invalid value "".  Not so clean (and just as
>      undocumented).
> 
> Here to deduplicate the two objects: keep @MigrationParameters as the name
> of object to use in both places, drop @MigrateSetParameters, at the
> meantime switch types of @tls* fields from "str" to "StrOrNull" types.
> 
> I found that the TLS code wasn't so much relying on tls_* fields being
> non-NULL at all.  Actually on the other way round: if we set tls_authz to
> an empty string (NOTE: currently, migrate_init() missed initializing
> tls_authz; also touched it up in this patch), we can already fail one of
> the migration-test (tls/x509/default-host), as qauthz_is_allowed_by_id()
> will assume tls_authz set even if tls_auths is an empty string.
> 
> It means we're actually relying on tls_* fields being NULL even if it's the
> empty string.
> 
> Let's just make it a rule to return NULL for empty string on these fields
> internally.  For that, when converting a StrOrNull into a char* (where we
> introduced a helper here in this patch) we'll also make the empty string to
> be NULL, to make it always work.  And it doesn't show any issue either when
> applying that logic to both tls_creds and tls_hostname.
> 
> With above, we can safely change both migration_tls_client_create() and
> migrate_tls() to not check the empty string too finally.. not needed
> anymore.
> 
> Also, we can drop the hackish conversions in qmp_migrate_set_parameters()
> where we want to make sure it's a QSTRING; it's not needed now.
> 
> This greatly deduplicates the code not only in qapi/migration.json, but
> also in the generic migration code.
> 
> Markus helped greatly with this patch.  Besides a better commit
> message (where I just "stole" from the reply), debugged and resolved a
> double free, but also provided the StrOrNull property implementation to be
> used in MigrationState object when switching tls_* fields to StrOrNull.
> 
> Co-developed-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  qapi/migration.json            | 191 +---------------------------
>  include/hw/qdev-properties.h   |   3 +
>  migration/options.h            |   3 +
>  hw/core/qdev-properties.c      |  40 ++++++
>  migration/migration-hmp-cmds.c |  20 +--
>  migration/options.c            | 220 ++++++++++-----------------------
>  migration/tls.c                |   3 +-
>  7 files changed, 125 insertions(+), 355 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|