Re: [RFC PATCH 05/13] migration: Reduce a bit of duplication in migration.json

["Daniel P. Berrangé" <berrange@redhat.com>]

On Fri, Apr 11, 2025 at 04:14:35PM -0300, Fabiano Rosas wrote:
> Introduce a new MigrationConfigBase, to allow most of the duplication
> in migration.json to be eliminated.
> 
> The reason we need MigrationParameters and MigrationSetParameters is
> that the internal parameter representation in the migration code, as
> well as the user-facing return of query-migrate-parameters use one
> type for the TLS options (tls-creds, tls-hostname, tls-authz), while
> the user-facing input from migrate-set-parameters uses another.
> 
> The difference is in whether the NULL values is accepted. The former
> considers NULL as invalid, while the latter doesn't.
> 
> Move all other (non-TLS) options into the new type and make it a base
> type for the two diverging types so that each child type can declare
> the TLS options in its own way.
> 
> Nothing changes in the user API, nothing changes in the internal
> representation, but we save several lines of duplication in
> migration.json.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  qapi/migration.json | 358 +++++++++++++-------------------------------
>  1 file changed, 108 insertions(+), 250 deletions(-)
> 
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 8b9c53595c..5a4d5a2d3e 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -914,202 +914,6 @@

> @@ -1277,45 +1059,121 @@
>  #     only has effect if the @mapped-ram capability is enabled.
>  #     (Since 9.1)
>  #
> +# @tls: Whether to use TLS. If this is set the options @tls-authz,
> +#     @tls-creds, @tls-hostname are mandatory and a valid string is
> +#     expected. (Since 10.1)
> +#

I'm not really finding it compelling to add a bool @tls as it
is just a denormalization of  !!@tls-creds.

Incidentally the docs here are wrong - TLS can be used by
only setting @tls-creds. The @tls-authz & @tls-hostname
params are always optional.

>  # Features:
>  #
>  # @unstable: Members @x-checkpoint-delay and
>  #     @x-vcpu-dirty-limit-period are experimental.
>  #
> +# Since: 10.1
> +##
> +{ 'struct': 'MigrationConfigBase',
> +  'data': { '*announce-initial': 'size',
> +            '*announce-max': 'size',
> +            '*announce-rounds': 'size',
> +            '*announce-step': 'size',
> +            '*throttle-trigger-threshold': 'uint8',
> +            '*cpu-throttle-initial': 'uint8',
> +            '*cpu-throttle-increment': 'uint8',
> +            '*cpu-throttle-tailslow': 'bool',
> +            '*max-bandwidth': 'size',
> +            '*avail-switchover-bandwidth': 'size',
> +            '*downtime-limit': 'uint64',
> +            '*x-checkpoint-delay': { 'type': 'uint32',
> +                                     'features': [ 'unstable' ] },
> +            '*multifd-channels': 'uint8',
> +            '*xbzrle-cache-size': 'size',
> +            '*max-postcopy-bandwidth': 'size',
> +            '*max-cpu-throttle': 'uint8',
> +            '*multifd-compression': 'MultiFDCompression',
> +            '*multifd-zlib-level': 'uint8',
> +            '*multifd-qatzip-level': 'uint8',
> +            '*multifd-zstd-level': 'uint8',
> +            '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ],
> +            '*x-vcpu-dirty-limit-period': { 'type': 'uint64',
> +                                            'features': [ 'unstable' ] },
> +            '*vcpu-dirty-limit': 'uint64',
> +            '*mode': 'MigMode',
> +            '*zero-page-detection': 'ZeroPageDetection',
> +            '*direct-io': 'bool',
> +            '*tls': 'bool' } }


>  { 'struct': 'MigrationParameters',
> +  'base': 'MigrationConfigBase',
> +  'data': { 'tls-creds': 'str',
> +            'tls-hostname': 'str',
> +            'tls-authz': 'str' } }

snip

> +{ 'struct': 'MigrateSetParameters',
> +  'base': 'MigrationConfigBase',
> +  'data': { '*tls-creds': 'StrOrNull',
> +            '*tls-hostname': 'StrOrNull',
> +            '*tls-authz': 'StrOrNull' } }

I recall we discussed this difference a year or two ago, but can't
recall what the outcome was.

Making the TLS params optional is a back compatible change for
MigrationParameters. I would think replacing 'str' with 'StrOrNull'
is also back compatible. So I'm wondering if we can't just unify
the sttructs fully for TLS, even if one usage scenario never actually
needs the "OrNull" bit nor needs the optionality 


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 :|