[Qemu-devel] [PATCH 0/3] Don't QAPI without need

[Qemu-devel] [PATCH 0/3] Don't QAPI without need

[Markus Armbruster]

These are pretty trivial, probably simplest if I take them through my
tree.

Markus Armbruster (3):
  replay: Define ReplayMode without QAPI
  COLO: Define COLOMode without QAPI
  block: Remove unused BlockDeviceMapEntry

 include/migration/colo.h |  6 ++++++
 include/sysemu/replay.h  |  6 ++++++
 qapi-schema.json         | 34 ----------------------------------
 qapi/block-core.json     | 29 -----------------------------
 4 files changed, 12 insertions(+), 63 deletions(-)

-- 
2.7.5

[Qemu-devel] [PATCH 1/3] replay: Define ReplayMode without QAPI

[Markus Armbruster]

ReplayMode is defined in the QAPI schema, but not used there.  Of the
stuff QAPI generates for it only the typedef is actually used.  Use of
QAPI is pointless and only complicates things, so don't.

Cc: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/sysemu/replay.h |  6 ++++++
 qapi-schema.json        | 18 ------------------
 2 files changed, 6 insertions(+), 18 deletions(-)

diff --git a/include/sysemu/replay.h b/include/sysemu/replay.h
index fa14d0e..621bc61 100644
--- a/include/sysemu/replay.h
+++ b/include/sysemu/replay.h
@@ -42,6 +42,12 @@ typedef enum ReplayCheckpoint ReplayCheckpoint;
 
 typedef struct ReplayNetState ReplayNetState;
 
+typedef enum {
+    REPLAY_MODE_NONE,           /* replay / record disabled */
+    REPLAY_MODE_RECORD,         /* recording to replay log */
+    REPLAY_MODE_PLAY,           /* replaying log */
+} ReplayMode;
+
 extern ReplayMode replay_mode;
 
 /* Name of the initial VM snapshot */
diff --git a/qapi-schema.json b/qapi-schema.json
index 9c6c3e1..9b6f6cb 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -6258,24 +6258,6 @@
 { 'include': 'qapi/rocker.json' }
 
 ##
-# @ReplayMode:
-#
-# Mode of the replay subsystem.
-#
-# @none: normal execution mode. Replay or record are not enabled.
-#
-# @record: record mode. All non-deterministic data is written into the
-#          replay log.
-#
-# @play: replay mode. Non-deterministic data required for system execution
-#        is read from the log.
-#
-# Since: 2.5
-##
-{ 'enum': 'ReplayMode',
-  'data': [ 'none', 'record', 'play' ] }
-
-##
 # @xen-load-devices-state:
 #
 # Load the state of all devices from file. The RAM and the block devices
-- 
2.7.5

Re: [Qemu-devel] [PATCH 1/3] replay: Define ReplayMode without QAPI

[Pavel Dovgalyuk]

> From: Markus Armbruster [mailto:armbru@redhat.com]
> ReplayMode is defined in the QAPI schema, but not used there.  Of the
> stuff QAPI generates for it only the typedef is actually used.  Use of
> QAPI is pointless and only complicates things, so don't.

This a bit useless now, but we plan to add more patches that include
QAPI commands for controlling and inspecting the record/replay process.

Pavel Dovgalyuk

> 
> Cc: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  include/sysemu/replay.h |  6 ++++++
>  qapi-schema.json        | 18 ------------------
>  2 files changed, 6 insertions(+), 18 deletions(-)
> 
> diff --git a/include/sysemu/replay.h b/include/sysemu/replay.h
> index fa14d0e..621bc61 100644
> --- a/include/sysemu/replay.h
> +++ b/include/sysemu/replay.h
> @@ -42,6 +42,12 @@ typedef enum ReplayCheckpoint ReplayCheckpoint;
> 
>  typedef struct ReplayNetState ReplayNetState;
> 
> +typedef enum {
> +    REPLAY_MODE_NONE,           /* replay / record disabled */
> +    REPLAY_MODE_RECORD,         /* recording to replay log */
> +    REPLAY_MODE_PLAY,           /* replaying log */
> +} ReplayMode;
> +
>  extern ReplayMode replay_mode;
> 
>  /* Name of the initial VM snapshot */
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 9c6c3e1..9b6f6cb 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -6258,24 +6258,6 @@
>  { 'include': 'qapi/rocker.json' }
> 
>  ##
> -# @ReplayMode:
> -#
> -# Mode of the replay subsystem.
> -#
> -# @none: normal execution mode. Replay or record are not enabled.
> -#
> -# @record: record mode. All non-deterministic data is written into the
> -#          replay log.
> -#
> -# @play: replay mode. Non-deterministic data required for system execution
> -#        is read from the log.
> -#
> -# Since: 2.5
> -##
> -{ 'enum': 'ReplayMode',
> -  'data': [ 'none', 'record', 'play' ] }
> -
> -##
>  # @xen-load-devices-state:
>  #
>  # Load the state of all devices from file. The RAM and the block devices
> --
> 2.7.5

Re: [Qemu-devel] [PATCH 1/3] replay: Define ReplayMode without QAPI

[Markus Armbruster]

"Pavel Dovgalyuk" <dovgaluk@ispras.ru> writes:

>> From: Markus Armbruster [mailto:armbru@redhat.com]
>> ReplayMode is defined in the QAPI schema, but not used there.  Of the
>> stuff QAPI generates for it only the typedef is actually used.  Use of
>> QAPI is pointless and only complicates things, so don't.
>
> This a bit useless now, but we plan to add more patches that include
> QAPI commands for controlling and inspecting the record/replay process.

I assume YAGNI until proven otherwise.

Posting patches putting ReplayMode to use could serve as proof.

Re: [Qemu-devel] [PATCH 1/3] replay: Define ReplayMode without QAPI

[Pavel Dovgalyuk]

> From: Markus Armbruster [mailto:armbru@redhat.com]
> "Pavel Dovgalyuk" <dovgaluk@ispras.ru> writes:
> 
> >> From: Markus Armbruster [mailto:armbru@redhat.com]
> >> ReplayMode is defined in the QAPI schema, but not used there.  Of the
> >> stuff QAPI generates for it only the typedef is actually used.  Use of
> >> QAPI is pointless and only complicates things, so don't.
> >
> > This a bit useless now, but we plan to add more patches that include
> > QAPI commands for controlling and inspecting the record/replay process.
> 
> I assume YAGNI until proven otherwise.
> 
> Posting patches putting ReplayMode to use could serve as proof.

This is a sample of not yet included patch.

https://lists.nongnu.org/archive/html/qemu-devel/2014-07/msg04278.html

Pavel Dovgalyuk

[Qemu-devel] [PATCH 2/3] COLO: Define COLOMode without QAPI

[Markus Armbruster]

COLOMode is defined in the QAPI schema, but not used there.  Of the
stuff QAPI generates for it only the typedef is actually used.  Use of
QAPI is pointless and only complicates things, so don't.

Cc: zhanghailiang <zhang.zhanghailiang@huawei.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/migration/colo.h |  6 ++++++
 qapi-schema.json         | 16 ----------------
 2 files changed, 6 insertions(+), 16 deletions(-)

diff --git a/include/migration/colo.h b/include/migration/colo.h
index ff9874e..5d7c500 100644
--- a/include/migration/colo.h
+++ b/include/migration/colo.h
@@ -26,6 +26,12 @@ void migration_incoming_exit_colo(void);
 void *colo_process_incoming_thread(void *opaque);
 bool migration_incoming_in_colo_state(void);
 
+typedef enum {
+    COLO_MODE_UNKNOWN,
+    COLO_MODE_PRIMARY,
+    COLO_MODE_SECONDARY,
+} COLOMode;
+
 COLOMode get_colo_mode(void);
 
 /* failover */
diff --git a/qapi-schema.json b/qapi-schema.json
index 9b6f6cb..3f0eb05 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1304,22 +1304,6 @@
             'vmstate-loaded' ] }
 
 ##
-# @COLOMode:
-#
-# The colo mode
-#
-# @unknown: unknown mode
-#
-# @primary: master side
-#
-# @secondary: slave side
-#
-# Since: 2.8
-##
-{ 'enum': 'COLOMode',
-  'data': [ 'unknown', 'primary', 'secondary'] }
-
-##
 # @FailoverStatus:
 #
 # An enumeration of COLO failover status
-- 
2.7.5

Re: [Qemu-devel] [PATCH 2/3] COLO: Define COLOMode without QAPI

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> COLOMode is defined in the QAPI schema, but not used there.  Of the
> stuff QAPI generates for it only the typedef is actually used.  Use of
> QAPI is pointless and only complicates things, so don't.

Hmm, in one of the old COLO worlds I have, there's code to emit an event
on exiting from COLO and that event includes the mode it was in.

If the intent is to bring that or similar back then it would be worth
keeping.

Dave

> Cc: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  include/migration/colo.h |  6 ++++++
>  qapi-schema.json         | 16 ----------------
>  2 files changed, 6 insertions(+), 16 deletions(-)
> 
> diff --git a/include/migration/colo.h b/include/migration/colo.h
> index ff9874e..5d7c500 100644
> --- a/include/migration/colo.h
> +++ b/include/migration/colo.h
> @@ -26,6 +26,12 @@ void migration_incoming_exit_colo(void);
>  void *colo_process_incoming_thread(void *opaque);
>  bool migration_incoming_in_colo_state(void);
>  
> +typedef enum {
> +    COLO_MODE_UNKNOWN,
> +    COLO_MODE_PRIMARY,
> +    COLO_MODE_SECONDARY,
> +} COLOMode;
> +
>  COLOMode get_colo_mode(void);
>  
>  /* failover */
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 9b6f6cb..3f0eb05 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1304,22 +1304,6 @@
>              'vmstate-loaded' ] }
>  
>  ##
> -# @COLOMode:
> -#
> -# The colo mode
> -#
> -# @unknown: unknown mode
> -#
> -# @primary: master side
> -#
> -# @secondary: slave side
> -#
> -# Since: 2.8
> -##
> -{ 'enum': 'COLOMode',
> -  'data': [ 'unknown', 'primary', 'secondary'] }
> -
> -##
>  # @FailoverStatus:
>  #
>  # An enumeration of COLO failover status
> -- 
> 2.7.5
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 2/3] COLO: Define COLOMode without QAPI

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 740 bytes --]

On 07/28/2017 12:17 PM, Dr. David Alan Gilbert wrote:
> * Markus Armbruster (armbru@redhat.com) wrote:
>> COLOMode is defined in the QAPI schema, but not used there.  Of the
>> stuff QAPI generates for it only the typedef is actually used.  Use of
>> QAPI is pointless and only complicates things, so don't.
> 
> Hmm, in one of the old COLO worlds I have, there's code to emit an event
> on exiting from COLO and that event includes the mode it was in.
> 
> If the intent is to bring that or similar back then it would be worth
> keeping.

We can always revert the removal once there is a user.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH 2/3] COLO: Define COLOMode without QAPI

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 07/28/2017 12:17 PM, Dr. David Alan Gilbert wrote:
>> * Markus Armbruster (armbru@redhat.com) wrote:
>>> COLOMode is defined in the QAPI schema, but not used there.  Of the
>>> stuff QAPI generates for it only the typedef is actually used.  Use of
>>> QAPI is pointless and only complicates things, so don't.
>> 
>> Hmm, in one of the old COLO worlds I have, there's code to emit an event
>> on exiting from COLO and that event includes the mode it was in.
>> 
>> If the intent is to bring that or similar back then it would be worth
>> keeping.
>
> We can always revert the removal once there is a user.

I assume YAGNI until proven otherwise.

Posting patches putting COLOMode to use could serve as proof.

Re: [Qemu-devel] [PATCH 2/3] COLO: Define COLOMode without QAPI

[Hailiang Zhang]

On 2017/7/29 1:17, Dr. David Alan Gilbert wrote:
> * Markus Armbruster (armbru@redhat.com) wrote:
>> COLOMode is defined in the QAPI schema, but not used there.  Of the
>> stuff QAPI generates for it only the typedef is actually used.  Use of
>> QAPI is pointless and only complicates things, so don't.
> Hmm, in one of the old COLO worlds I have, there's code to emit an event
> on exiting from COLO and that event includes the mode it was in.

Yes, we need it in the later series.

> If the intent is to bring that or similar back then it would be worth
> keeping.

Agreed.  ;)

> Dave
>
>> Cc: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>   include/migration/colo.h |  6 ++++++
>>   qapi-schema.json         | 16 ----------------
>>   2 files changed, 6 insertions(+), 16 deletions(-)
>>
>> diff --git a/include/migration/colo.h b/include/migration/colo.h
>> index ff9874e..5d7c500 100644
>> --- a/include/migration/colo.h
>> +++ b/include/migration/colo.h
>> @@ -26,6 +26,12 @@ void migration_incoming_exit_colo(void);
>>   void *colo_process_incoming_thread(void *opaque);
>>   bool migration_incoming_in_colo_state(void);
>>   
>> +typedef enum {
>> +    COLO_MODE_UNKNOWN,
>> +    COLO_MODE_PRIMARY,
>> +    COLO_MODE_SECONDARY,
>> +} COLOMode;
>> +
>>   COLOMode get_colo_mode(void);
>>   
>>   /* failover */
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index 9b6f6cb..3f0eb05 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -1304,22 +1304,6 @@
>>               'vmstate-loaded' ] }
>>   
>>   ##
>> -# @COLOMode:
>> -#
>> -# The colo mode
>> -#
>> -# @unknown: unknown mode
>> -#
>> -# @primary: master side
>> -#
>> -# @secondary: slave side
>> -#
>> -# Since: 2.8
>> -##
>> -{ 'enum': 'COLOMode',
>> -  'data': [ 'unknown', 'primary', 'secondary'] }
>> -
>> -##
>>   # @FailoverStatus:
>>   #
>>   # An enumeration of COLO failover status
>> -- 
>> 2.7.5
>>
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>
> .
>

[Qemu-devel] [PATCH 3/3] block: Remove unused BlockDeviceMapEntry

[Markus Armbruster]

BlockDeviceMapEntry has never been used.  It was added in commit
facd6e2 "so that it is published through the introspection mechanism."
What exactly introspecting types that aren't used for anything could
accomplish isn't clear.  What "introspection mechanism" to use is also
nebulous.  To the best of my knowledge, there has never been one that
covered this type.  Certainly not query-qmp-schema, which includes
only types that are actually used in QMP.

Not being able to introspect BlockDeviceMapEntry hasn't bothered
anyone enough to complain in almost four years.  Get rid of it.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Eric Blake <eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qapi/block-core.json | 29 -----------------------------
 1 file changed, 29 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 833c602..27790f3 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -383,35 +383,6 @@
 { 'enum': 'BlockDeviceIoStatus', 'data': [ 'ok', 'failed', 'nospace' ] }
 
 ##
-# @BlockDeviceMapEntry:
-#
-# Entry in the metadata map of the device (returned by "qemu-img map")
-#
-# @start: Offset in the image of the first byte described by this entry
-#         (in bytes)
-#
-# @length: Length of the range described by this entry (in bytes)
-#
-# @depth: Number of layers (0 = top image, 1 = top image's backing file, etc.)
-#         before reaching one for which the range is allocated.  The value is
-#         in the range 0 to the depth of the image chain - 1.
-#
-# @zero: the sectors in this range read as zeros
-#
-# @data: reading the image will actually read data from a file (in particular,
-#        if @offset is present this means that the sectors are not simply
-#        preallocated, but contain actual data in raw format)
-#
-# @offset: if present, the image file stores the data for this range in
-#          raw format at the given offset.
-#
-# Since: 1.7
-##
-{ 'struct': 'BlockDeviceMapEntry',
-  'data': { 'start': 'int', 'length': 'int', 'depth': 'int', 'zero': 'bool',
-            'data': 'bool', '*offset': 'int' } }
-
-##
 # @DirtyBitmapStatus:
 #
 # An enumeration of possible states that a dirty bitmap can report to the user.
-- 
2.7.5

Re: [Qemu-devel] [PATCH 3/3] block: Remove unused BlockDeviceMapEntry

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1274 bytes --]

On 07/28/2017 08:45 AM, Markus Armbruster wrote:
> BlockDeviceMapEntry has never been used.  It was added in commit
> facd6e2 "so that it is published through the introspection mechanism."
> What exactly introspecting types that aren't used for anything could
> accomplish isn't clear.  What "introspection mechanism" to use is also
> nebulous.  To the best of my knowledge, there has never been one that
> covered this type.  Certainly not query-qmp-schema, which includes
> only types that are actually used in QMP.
> 
> Not being able to introspect BlockDeviceMapEntry hasn't bothered
> anyone enough to complain in almost four years.  Get rid of it.

This type is the schema for 'qemu-img map --output=json'.  And I had a
patch once (that I need to revive) that added a JSON Output visitor; at
which point I fixed qemu-img to convert from QAPI to JSON instead of
open-coding its construction of its output string, at which point the
QAPI generated code for this type is useful.

But that's a revert away for when I rebase my series to actually use it;
and in the meantime, pruning unused types isn't hurting anything.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH 3/3] block: Remove unused BlockDeviceMapEntry

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1540 bytes --]

On 2017-07-28 20:10, Eric Blake wrote:
> On 07/28/2017 08:45 AM, Markus Armbruster wrote:
>> BlockDeviceMapEntry has never been used.  It was added in commit
>> facd6e2 "so that it is published through the introspection mechanism."
>> What exactly introspecting types that aren't used for anything could
>> accomplish isn't clear.  What "introspection mechanism" to use is also
>> nebulous.  To the best of my knowledge, there has never been one that
>> covered this type.  Certainly not query-qmp-schema, which includes
>> only types that are actually used in QMP.
>>
>> Not being able to introspect BlockDeviceMapEntry hasn't bothered
>> anyone enough to complain in almost four years.  Get rid of it.
> 
> This type is the schema for 'qemu-img map --output=json'.  And I had a
> patch once (that I need to revive) that added a JSON Output visitor; at
> which point I fixed qemu-img to convert from QAPI to JSON instead of
> open-coding its construction of its output string, at which point the
> QAPI generated code for this type is useful.
(Very late reply, I know, I just stumbled over *MapEntry when looking
over block-core.json what we might want to deprecate in 3.0)

We already use MapEntry there -- why don't we output just that instead?
The only difference seems to be an additional @filename parameter which
would probably be actually nice to include in the output.

Except that BlockDeviceMapEntry's documentation is better, so we should
merge that into MapEntry before removing the former.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH 0/3] Don't QAPI without need

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 751 bytes --]

On 07/28/2017 08:45 AM, Markus Armbruster wrote:
> These are pretty trivial, probably simplest if I take them through my
> tree.
> 
> Markus Armbruster (3):
>   replay: Define ReplayMode without QAPI
>   COLO: Define COLOMode without QAPI
>   block: Remove unused BlockDeviceMapEntry
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

>  include/migration/colo.h |  6 ++++++
>  include/sysemu/replay.h  |  6 ++++++
>  qapi-schema.json         | 34 ----------------------------------
>  qapi/block-core.json     | 29 -----------------------------
>  4 files changed, 12 insertions(+), 63 deletions(-)
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]