Re: [PATCH RFC 02/12] migration/qapi: Rename MigrationStats to MigrationRAMStats

[Markus Armbruster <armbru@redhat.com>]

Cc: correct libvirt list address

Peter Xu <peterx@redhat.com> writes:

> This stats is only about RAM, so make it accurate.  This paves way for
> statistics for all devices.
>
> Note that even if this is part of qapi/, this should not be part of ABI of

docs/devel/qapi-code-gen.rst section "Compatibility considerations":

    Since type names are not visible in the Client JSON Protocol, types
    may be freely renamed.  Even certain refactorings are invisible, such
    as splitting members from one type into a common base type.

So, s/should not be/is not/.

> at least query-migrate, because the structure is not changed, and this
> stats is always reported only under the "ram" section.

Why "at least query-migrate"?  Do you have other interfaces in mind, or
are you just hedging?

> Cc: Daniel P. Berrangé <berrange@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Libvirt Mailing List <libvir-list@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  docs/about/removed-features.rst | 2 +-
>  qapi/migration.json             | 8 ++++----
>  migration/migration-stats.h     | 2 +-
>  3 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/docs/about/removed-features.rst b/docs/about/removed-features.rst
> index 6f4447993c..7c4f4325f7 100644
> --- a/docs/about/removed-features.rst
> +++ b/docs/about/removed-features.rst
> @@ -699,7 +699,7 @@ was superseded by ``sections``.
>  ``query-migrate`` return value member ``skipped`` (removed in 9.1)
>  ''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
>  
> -Member ``skipped`` of the ``MigrationStats`` struct hasn't been used
> +Member ``skipped`` of the ``MigrationRAMStats`` struct hasn't been used
>  for more than 10 years. Removed with no replacement.

Could simply use "of the return value".  But this is fine, too.

>  
>  ``migrate`` command option ``inc`` (removed in 9.1)
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 7134d4ce47..cfc6ccee26 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -12,7 +12,7 @@
>  { 'include': 'sockets.json' }
>  
>  ##
> -# @MigrationStats:
> +# @MigrationRAMStats:
>  #
>  # Detailed migration status.
>  #
> @@ -64,7 +64,7 @@
>  #
>  # Since: 0.14
>  ##
> -{ 'struct': 'MigrationStats',
> +{ 'struct': 'MigrationRAMStats',
>    'data': {'transferred': 'int', 'remaining': 'int', 'total': 'int' ,
>             'duplicate': 'int',
>             'normal': 'int',
> @@ -209,7 +209,7 @@
>  #     If this field is not returned, no migration process has been
>  #     initiated
>  #
> -# @ram: `MigrationStats` containing detailed migration status, only
> +# @ram: `MigrationRAMStats` containing detailed migration status, only
>  #     returned if status is 'active' or 'completed'(since 1.2)

This gets rendered like

      * ram (MigrationStats, *optional*) -- MigrationStats
        containing detailed migration status, only returned if status
        is 'active' or 'completed'(since 1.2)

Recommend to rephrase to avoid the repetition.  Maybe

   # @ram: Detailed migration RAM status, only returned if status is
   #     'active' or 'completed' (since 1.2)

Also consider replacing "status" by "statistics".

>  #
>  # @xbzrle-cache: `XBZRLECacheStats` containing detailed XBZRLE
> @@ -309,7 +309,7 @@
>  # Since: 0.14
>  ##
>  { 'struct': 'MigrationInfo',
> -  'data': {'*status': 'MigrationStatus', '*ram': 'MigrationStats',
> +  'data': {'*status': 'MigrationStatus', '*ram': 'MigrationRAMStats',
>             '*vfio': 'VfioStats',
>             '*xbzrle-cache': 'XBZRLECacheStats',
>             '*total-time': 'int',
> diff --git a/migration/migration-stats.h b/migration/migration-stats.h
> index c0f50144c9..1153520f7a 100644
> --- a/migration/migration-stats.h
> +++ b/migration/migration-stats.h
> @@ -27,7 +27,7 @@
>  
>  /*
>   * These are the ram migration statistic counters.  It is loosely
> - * based on MigrationStats.
> + * based on MigrationRAMStats.
>   */
>  typedef struct {
>      /*

Only minor issues, and none added by this patch, so
Acked-by: Markus Armbruster <armbru@redhat.com>