Re: [PATCH RFC 02/12] migration/qapi: Rename MigrationStats to MigrationRAMStats
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>
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
On Fri, Mar 20, 2026 at 07:54:38AM +0100, Markus Armbruster wrote:
Recommend to rephrase to avoid the repetition. Maybe
# @ram: Detailed migration RAM status, only returned if status is # 'active' or 'completed' (since 1.2)
I just noticed the previous document was also wrong.. it was too ancient since 1.2, and we introduced too many new status that also reports RAM statistics.. I changed it to below: # @ram: Detailed migration RAM statistics, only returned if migration # is in progress or completed (since 1.2) -- Peter Xu
participants (2)
-
Markus Armbruster -
Peter Xu