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

[Peter Xu <peterx@redhat.com>]

On Fri, Mar 20, 2026 at 07:54:38AM +0100, Markus Armbruster wrote:
> 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?

I wanted to say the QMP command that consumes this structure will not be
affected.  Happy to learn that it's explicitly described in qapi document,
thanks for the pointer!  I'll also add this reference in the commit
message.

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

Let me keep this as-is then; the plan is to make this patch as simple as a
rename effort, as it's not accurate anymore (e.g. dirty-sync-count should
be a global statistic rather than RAM-only), and it also makes that name
available for system-wise statistics.

> 
> >  
> >  ``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".

Sure, I'll touch it up while at this.

> 
> >  #
> >  # @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>

It took me a while to be able to go back to this series, thanks for the
super fast feedback!

-- 
Peter Xu