Re: [PATCH v3 1/4] migration: migrate 'inc' command option is deprecated.

[Markus Armbruster <armbru@redhat.com>]

Juan Quintela <quintela@redhat.com> writes:

> Set the 'block_incremental' migration parameter to 'true' instead.
>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  docs/about/deprecated.rst |  7 +++++++
>  qapi/migration.json       | 12 ++++++++++--
>  migration/migration.c     |  6 ++++++
>  3 files changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index 8b136320e2..eb0f326f00 100644
> --- a/docs/about/deprecated.rst
> +++ b/docs/about/deprecated.rst
> @@ -417,3 +417,10 @@ Migration
>  ``skipped`` field in Migration stats has been deprecated.  It hasn't
>  been used for more than 10 years.
>  
> +``inc`` migrate command option (since 8.2)
> +''''''''''''''''''''''''''''''''''''''''''
> +
> +The new way to modify migration is using migration parameters.
> +``inc`` functionality can be achieved by setting the
> +``block-incremental`` migration parameter to ``true``.
> +
> diff --git a/qapi/migration.json b/qapi/migration.json
> index d7dfaa5db9..7669c98c7a 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1486,13 +1486,20 @@
>  #
>  # @blk: do block migration (full disk copy)
>  #
> -# @inc: incremental disk copy migration
> +# @inc: incremental disk copy migration.  This option is deprecated.
> +#     Set the 'block-incremetantal' migration parameter to 'true'
> +#     instead.

'block-incremental'

>  #
>  # @detach: this argument exists only for compatibility reasons and is
>  #     ignored by QEMU
>  #
>  # @resume: resume one paused migration, default "off". (since 3.0)
>  #
> +# Features:
> +#
> +# @deprecated: option @inc should be enabled by setting the
> +#     'block-incremental' migration parameter to 'true'.
> +#

You add deprecation notices, one to the member documentation, and one to
the "Features:" section.  You should add just one, to the "Features:"
section.  Suggest:

   # @deprecated: Member @inc is deprecated.  Use migration parameter
   # @block-incremental instead.

>  # Returns: nothing on success
>  #
>  # Since: 0.14
> @@ -1514,7 +1521,8 @@
>  # <- { "return": {} }
>  ##
>  { 'command': 'migrate',
> -  'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool',
> +  'data': {'uri': 'str', '*blk': 'bool',
> +           '*inc': { 'type': 'bool', 'features': ['deprecated'] },

For better or worse, we format like [ 'deprecated' ].

>             '*detach': 'bool', '*resume': 'bool' } }
>  
>  ##
> diff --git a/migration/migration.c b/migration/migration.c
> index 1c6c81ad49..c7e4c37b8a 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1601,6 +1601,12 @@ static bool migrate_prepare(MigrationState *s, bool blk, bool blk_inc,
>  {
>      Error *local_err = NULL;
>  
> +    if (blk_inc) {
> +        warn_report("-inc migrate option is deprecated, set the "
> +                    "'block-incremental' migration parameter to 'true'"
> +                    " instead.");

There is no "-inc migrate option".  You're refering to QMP command
migrate's parameter @inc / HMP command migrate's flag -i.

> +    }
> +
>      if (resume) {
>          if (s->state != MIGRATION_STATUS_POSTCOPY_PAUSED) {
>              error_setg(errp, "Cannot resume if there is no "

As far as I can see, HMP command migrate still uses the deprecated
interface:

    qmp_migrate(uri, !!blk, blk, !!inc, inc,
                false, false, true, resume, &err);

Its use should be replaced before we deprecate it.