[PATCH v2 00/14] Fix some qapi examples and a TODO section

[PATCH v2 00/14] Fix some qapi examples and a TODO section

[Victor Toso]

I've being using the examples as unit tests and found a few that
doesn't work out-of-the-box, might be inteded in order to be less
verbose in the qapi documentation but nevertheless I'm sending this
out in case you want to cherry-pick them.

* Changes in v2:
02/14: qapi: fix example of BLOCK_IMAGE_CORRUPTED event
    - changed the example to an actual error picked from
      qemu-iotests/60.out
    - added recent timestamp :)
03/14: qapi: fix example of BLOCK_IO_ERROR event
    - use real error message, in this case "No space left on device"
04/14: qapi: fix example of BLOCK_JOB_PENDING event
    - added an extra space
    - added recent timestamp
07/14: qapi: fix example of UNPLUG_PRIMARY event
    - amended commit message
08/14: qapi: fix example of FAILOVER_NEGOTIATED event
    - fixed syntax for proper document parameter
09/14: qapi: run-state examples: add missing member
    - fixed commit message
10/14: qapi: run-state examples: add missing timestamp
    - added chronological timestamps ;)

Cheers,

Victor Toso (14):
  qapi: BlockExportRemoveMode: move comments to TODO
  qapi: fix example of BLOCK_IMAGE_CORRUPTED event
  qapi: fix example of BLOCK_IO_ERROR event
  qapi: fix example of BLOCK_JOB_PENDING event
  qapi: fix example of DUMP_COMPLETED event
  qapi: fix example of MEMORY_DEVICE_SIZE_CHANGE event
  qapi: fix example of UNPLUG_PRIMARY event
  qapi: fix example of FAILOVER_NEGOTIATED event
  qapi: fix examples: SHUTDOWN and RESET events
  qapi: run-state examples: add missing timestamp
  qapi: fix example of MEMORY_FAILURE
  qapi: ui examples: add missing websocket member
  qapi: fix example of ACPI_DEVICE_OST event
  qapi: fix example of dump-guest-memory

 qapi/acpi.json         |  5 +++--
 qapi/block-core.json   | 14 +++++++-------
 qapi/block-export.json | 10 +++++-----
 qapi/dump.json         |  9 +++++----
 qapi/machine.json      |  3 ++-
 qapi/migration.json    |  4 +++-
 qapi/net.json          |  6 ++++--
 qapi/run-state.json    | 16 +++++++++++-----
 qapi/ui.json           | 12 ++++++------
 9 files changed, 46 insertions(+), 33 deletions(-)

-- 
2.35.1

[PATCH v2 01/14] qapi: BlockExportRemoveMode: move comments to TODO

[Victor Toso]

@hide and @soft are potential additions which fits the TODO section
perfectly.

The main motivation is to avoid this whole block of comment entering
the wrong section in the python parser.

Signed-off-by: Victor Toso <victortoso@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
 qapi/block-export.json | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/qapi/block-export.json b/qapi/block-export.json
index f183522d0d..1e34927f85 100644
--- a/qapi/block-export.json
+++ b/qapi/block-export.json
@@ -219,13 +219,13 @@
 #
 # @hard: Drop all connections immediately and remove export.
 #
-# Potential additional modes to be added in the future:
+# TODO: Potential additional modes to be added in the future:
 #
-# hide: Just hide export from new clients, leave existing connections as is.
-# Remove export after all clients are disconnected.
+#       hide: Just hide export from new clients, leave existing connections as is.
+#       Remove export after all clients are disconnected.
 #
-# soft: Hide export from new clients, answer with ESHUTDOWN for all further
-# requests from existing clients.
+#       soft: Hide export from new clients, answer with ESHUTDOWN for all further
+#       requests from existing clients.
 #
 # Since: 2.12
 ##
-- 
2.35.1

[PATCH v2 02/14] qapi: fix example of BLOCK_IMAGE_CORRUPTED event

[Victor Toso]

Fatal is not optional.

Signed-off-by: Victor Toso <victortoso@redhat.com>
---
 qapi/block-core.json | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index e89f2dfb5b..63c30a5378 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -5006,10 +5006,9 @@
 # Example:
 #
 # <- { "event": "BLOCK_IMAGE_CORRUPTED",
-#      "data": { "device": "ide0-hd0", "node-name": "node0",
-#                "msg": "Prevented active L1 table overwrite", "offset": 196608,
-#                "size": 65536 },
-#      "timestamp": { "seconds": 1378126126, "microseconds": 966463 } }
+#      "data": { "device": "", "node-name": "drive", "fatal": false,
+#                "msg": "L2 table offset 0x2a2a2a00 unaligned (L1 index: 0)" },
+#      "timestamp": { "seconds": 1648243240, "microseconds": 906060 } }
 #
 # Since: 1.7
 ##
-- 
2.35.1

[PATCH v2 03/14] qapi: fix example of BLOCK_IO_ERROR event

[Victor Toso]

Reason is not optional.

Signed-off-by: Victor Toso <victortoso@redhat.com>
---
 qapi/block-core.json | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 63c30a5378..46580ac551 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -5058,7 +5058,8 @@
 #      "data": { "device": "ide0-hd1",
 #                "node-name": "#block212",
 #                "operation": "write",
-#                "action": "stop" },
+#                "action": "stop",
+#                "reason": "No space left on device" },
 #      "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
 #
 ##
-- 
2.35.1

[PATCH v2 04/14] qapi: fix example of BLOCK_JOB_PENDING event

[Victor Toso]

* Event's name: BLOCK_JOB_WAITING -> BLOCK_JOB_PENDING
* Argument device -> id

Signed-off-by: Victor Toso <victortoso@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 qapi/block-core.json | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 46580ac551..4a7a6940a3 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -5225,8 +5225,8 @@
 #
 # Example:
 #
-# <- { "event": "BLOCK_JOB_WAITING",
-#      "data": { "device": "drive0", "type": "mirror" },
+# <- { "event": "BLOCK_JOB_PENDING",
+#      "data": { "type": "mirror", "id": "backup_1" },
 #      "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
 #
 ##
-- 
2.35.1

[PATCH v2 05/14] qapi: fix example of DUMP_COMPLETED event

[Victor Toso]

* Timestamp is not optional, let's add for completeness.
* Add '<-' to signalize it is receiving the data
* While at it, add extra space before "result" and "total"
  and after "completed" value.

Signed-off-by: Victor Toso <victortoso@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
 qapi/dump.json | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/qapi/dump.json b/qapi/dump.json
index f7c4267e3f..9119c82b14 100644
--- a/qapi/dump.json
+++ b/qapi/dump.json
@@ -161,9 +161,10 @@
 #
 # Example:
 #
-# { "event": "DUMP_COMPLETED",
-#   "data": {"result": {"total": 1090650112, "status": "completed",
-#                       "completed": 1090650112} } }
+# <- { "event": "DUMP_COMPLETED",
+#      "data": { "result": { "total": 1090650112, "status": "completed",
+#                            "completed": 1090650112 } },
+#      "timestamp": { "seconds": 1648244171, "microseconds": 950316 } }
 #
 ##
 { 'event': 'DUMP_COMPLETED' ,
-- 
2.35.1

[PATCH v2 06/14] qapi: fix example of MEMORY_DEVICE_SIZE_CHANGE event

[Victor Toso]

* qom-path is not optional

Signed-off-by: Victor Toso <victortoso@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 qapi/machine.json | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/qapi/machine.json b/qapi/machine.json
index 42fc68403d..9c460ec450 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1356,7 +1356,8 @@
 # Example:
 #
 # <- { "event": "MEMORY_DEVICE_SIZE_CHANGE",
-#      "data": { "id": "vm0", "size": 1073741824},
+#      "data": { "id": "vm0", "size": 1073741824,
+#                "qom-path": "/machine/unattached/device[2]" },
 #      "timestamp": { "seconds": 1588168529, "microseconds": 201316 } }
 #
 ##
-- 
2.35.1

[PATCH v2 07/14] qapi: fix example of UNPLUG_PRIMARY event

[Victor Toso]

* Timestamp is not optional, let's add for completeness.
* Add '<-' to signalize it is receiving the data
* Break lines like most of examples do

Signed-off-by: Victor Toso <victortoso@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
 qapi/migration.json | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/qapi/migration.json b/qapi/migration.json
index 18e2610e88..092a63354b 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1736,7 +1736,9 @@
 # Since: 4.2
 #
 # Example:
-#   {"event": "UNPLUG_PRIMARY", "data": {"device-id": "hostdev0"} }
+# <- { "event": "UNPLUG_PRIMARY",
+#      "data": { "device-id": "hostdev0" },
+#      "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
 #
 ##
 { 'event': 'UNPLUG_PRIMARY',
-- 
2.35.1

[PATCH v2 08/14] qapi: fix example of FAILOVER_NEGOTIATED event

[Victor Toso]

* Data is an object, not a string. It generates a qdict.
* Timestamp is not optional, let's add for completeness.
* Add missing "@" to proper document "device-id"
* Add extra space for "Since" section

Signed-off-by: Victor Toso <victortoso@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
 qapi/net.json | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/qapi/net.json b/qapi/net.json
index 7fab2e7cd8..0d4578bd07 100644
--- a/qapi/net.json
+++ b/qapi/net.json
@@ -685,13 +685,15 @@
 # Failover primary devices which were hidden (not hotplugged when requested)
 # before will now be hotplugged by the virtio-net standby device.
 #
-# device-id: QEMU device id of the unplugged device
+# @device-id: QEMU device id of the unplugged device
+#
 # Since: 4.2
 #
 # Example:
 #
 # <- { "event": "FAILOVER_NEGOTIATED",
-#      "data": "net1" }
+#      "data": { "device-id": "net1" },
+#      "timestamp": { "seconds": 1368697518, "microseconds": 326866 } }
 #
 ##
 { 'event': 'FAILOVER_NEGOTIATED',
-- 
2.35.1

[PATCH v2 09/14] qapi: fix examples: SHUTDOWN and RESET events

[Victor Toso]

The 'reason' member in not optional.

Signed-off-by: Victor Toso <victortoso@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
 qapi/run-state.json | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/qapi/run-state.json b/qapi/run-state.json
index 43d66d700f..1b9f64c9cd 100644
--- a/qapi/run-state.json
+++ b/qapi/run-state.json
@@ -150,7 +150,8 @@
 #
 # Example:
 #
-# <- { "event": "SHUTDOWN", "data": { "guest": true },
+# <- { "event": "SHUTDOWN",
+#      "data": { "guest": true, "reason": "guest-shutdown" },
 #      "timestamp": { "seconds": 1267040730, "microseconds": 682951 } }
 #
 ##
@@ -188,7 +189,8 @@
 #
 # Example:
 #
-# <- { "event": "RESET", "data": { "guest": false },
+# <- { "event": "RESET",
+#      "data": { "guest": false, "reason": "guest-reset" },
 #      "timestamp": { "seconds": 1267041653, "microseconds": 9518 } }
 #
 ##
-- 
2.35.1

[PATCH v2 10/14] qapi: run-state examples: add missing timestamp

[Victor Toso]

Signed-off-by: Victor Toso <victortoso@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
 qapi/run-state.json | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/qapi/run-state.json b/qapi/run-state.json
index 1b9f64c9cd..7f1c788c4e 100644
--- a/qapi/run-state.json
+++ b/qapi/run-state.json
@@ -426,7 +426,8 @@
 # Example:
 #
 # <- { "event": "GUEST_PANICKED",
-#      "data": { "action": "pause" } }
+#      "data": { "action": "pause" },
+#      "timestamp": { "seconds": 1648245231, "microseconds": 900001 } }
 #
 ##
 { 'event': 'GUEST_PANICKED',
@@ -446,7 +447,8 @@
 # Example:
 #
 # <- { "event": "GUEST_CRASHLOADED",
-#      "data": { "action": "run" } }
+#      "data": { "action": "run" },
+#      "timestamp": { "seconds": 1648245259, "microseconds": 893771 } }
 #
 ##
 { 'event': 'GUEST_CRASHLOADED',
-- 
2.35.1

[PATCH v2 11/14] qapi: fix example of MEMORY_FAILURE

[Victor Toso]

Minor issues found and fixed with the example:
* The JSON object of EVENT was not closed
* Missing timestamp
* Flags are optional but if defined then all members should be
  include so we add "recursive" member.
* Changed string from '' to "" in action-required member.

Signed-off-by: Victor Toso <victortoso@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
 qapi/run-state.json | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/qapi/run-state.json b/qapi/run-state.json
index 7f1c788c4e..8124220bd9 100644
--- a/qapi/run-state.json
+++ b/qapi/run-state.json
@@ -571,7 +571,9 @@
 # <- { "event": "MEMORY_FAILURE",
 #      "data": { "recipient": "hypervisor",
 #                "action": "fatal",
-#                "flags": { 'action-required': false } }
+#                "flags": { "action-required": false,
+#                           "recursive": false } },
+#      "timestamp": { "seconds": 1267061043, "microseconds": 959568 } }
 #
 ##
 { 'event': 'MEMORY_FAILURE',
-- 
2.35.1

[PATCH v2 12/14] qapi: ui examples: add missing websocket member

[Victor Toso]

As the websocket is not optional in VncBasicInfo.

Signed-off-by: Victor Toso <victortoso@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
 qapi/ui.json | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/qapi/ui.json b/qapi/ui.json
index 664da9e462..a810ed680c 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -710,10 +710,10 @@
 #
 # <- { "event": "VNC_CONNECTED",
 #      "data": {
-#            "server": { "auth": "sasl", "family": "ipv4",
+#            "server": { "auth": "sasl", "family": "ipv4", "websocket": false,
 #                        "service": "5901", "host": "0.0.0.0" },
 #            "client": { "family": "ipv4", "service": "58425",
-#                        "host": "127.0.0.1" } },
+#                        "host": "127.0.0.1", "websocket": false } },
 #      "timestamp": { "seconds": 1262976601, "microseconds": 975795 } }
 #
 ##
@@ -738,9 +738,9 @@
 #
 # <-  { "event": "VNC_INITIALIZED",
 #       "data": {
-#            "server": { "auth": "sasl", "family": "ipv4",
+#            "server": { "auth": "sasl", "family": "ipv4", "websocket": false,
 #                        "service": "5901", "host": "0.0.0.0"},
-#            "client": { "family": "ipv4", "service": "46089",
+#            "client": { "family": "ipv4", "service": "46089", "websocket": false,
 #                        "host": "127.0.0.1", "sasl_username": "luiz" } },
 #       "timestamp": { "seconds": 1263475302, "microseconds": 150772 } }
 #
@@ -765,9 +765,9 @@
 #
 # <- { "event": "VNC_DISCONNECTED",
 #      "data": {
-#            "server": { "auth": "sasl", "family": "ipv4",
+#            "server": { "auth": "sasl", "family": "ipv4", "websocket": false,
 #                        "service": "5901", "host": "0.0.0.0" },
-#            "client": { "family": "ipv4", "service": "58425",
+#            "client": { "family": "ipv4", "service": "58425", "websocket": false,
 #                        "host": "127.0.0.1", "sasl_username": "luiz" } },
 #      "timestamp": { "seconds": 1262976601, "microseconds": 975795 } }
 #
-- 
2.35.1

[PATCH v2 13/14] qapi: fix example of ACPI_DEVICE_OST event

[Victor Toso]

* Missing the timestamp
* Missing the "info" object in the "data" member

Signed-off-by: Victor Toso <victortoso@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
 qapi/acpi.json | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/qapi/acpi.json b/qapi/acpi.json
index 51f0d55db7..d148f6db9f 100644
--- a/qapi/acpi.json
+++ b/qapi/acpi.json
@@ -133,8 +133,9 @@
 # Example:
 #
 # <- { "event": "ACPI_DEVICE_OST",
-#      "data": { "device": "d1", "slot": "0",
-#                "slot-type": "DIMM", "source": 1, "status": 0 } }
+#      "data": { "info": { "device": "d1", "slot": "0",
+#                          "slot-type": "DIMM", "source": 1, "status": 0 } },
+#      "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
 #
 ##
 { 'event': 'ACPI_DEVICE_OST',
-- 
2.35.1

[PATCH v2 14/14] qapi: fix example of dump-guest-memory

[Victor Toso]

The member "paging" is not optional

Signed-off-by: Victor Toso <victortoso@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
 qapi/dump.json | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qapi/dump.json b/qapi/dump.json
index 9119c82b14..29441af9d8 100644
--- a/qapi/dump.json
+++ b/qapi/dump.json
@@ -83,7 +83,7 @@
 # Example:
 #
 # -> { "execute": "dump-guest-memory",
-#      "arguments": { "protocol": "fd:dump" } }
+#      "arguments": { "paging": false, "protocol": "fd:dump" } }
 # <- { "return": {} }
 #
 ##
-- 
2.35.1

Re: [PATCH v2 02/14] qapi: fix example of BLOCK_IMAGE_CORRUPTED event

[Markus Armbruster]

Victor Toso <victortoso@redhat.com> writes:

> Fatal is not optional.

Recognizing that "Fatal" refers to member "fatal" takes mental effort.
Always use identifiers verbatim, including case.  Where that makes for
confusing prose, prefix with a @ like @fatal, or rephrase, like "The
member fatal".

>
> Signed-off-by: Victor Toso <victortoso@redhat.com>
> ---
>  qapi/block-core.json | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index e89f2dfb5b..63c30a5378 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -5006,10 +5006,9 @@
>  # Example:
>  #
>  # <- { "event": "BLOCK_IMAGE_CORRUPTED",
> -#      "data": { "device": "ide0-hd0", "node-name": "node0",
> -#                "msg": "Prevented active L1 table overwrite", "offset": 196608,
> -#                "size": 65536 },
> -#      "timestamp": { "seconds": 1378126126, "microseconds": 966463 } }
> +#      "data": { "device": "", "node-name": "drive", "fatal": false,
> +#                "msg": "L2 table offset 0x2a2a2a00 unaligned (L1 index: 0)" },
> +#      "timestamp": { "seconds": 1648243240, "microseconds": 906060 } }
>  #
>  # Since: 1.7
>  ##

Changing the value of @msg makes sense, but is worth a note in the
commit message.

Here's my try:

    qapi: fix example of BLOCK_IMAGE_CORRUPTED event

    Example output lacks mandatory member @fatal.  Provide it.

    Example output shows a value of @msg no version of the code
    produces.  No big deal, but replace it anyway by one that today's
    code does produce.

How do you like it?

[PATCH v2.1] qapi: fix example of BLOCK_IMAGE_CORRUPTED event

[Victor Toso]

Example output lacks mandatory member @fatal.  Provide it.

Example output shows a value of @msg no version of the code
produces.  No big deal, but replace it anyway by one that
today's code does produce.

Signed-off-by: Victor Toso <victortoso@redhat.com>
---
 qapi/block-core.json | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index e89f2dfb5b..63c30a5378 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -5006,10 +5006,9 @@
 # Example:
 #
 # <- { "event": "BLOCK_IMAGE_CORRUPTED",
-#      "data": { "device": "ide0-hd0", "node-name": "node0",
-#                "msg": "Prevented active L1 table overwrite", "offset": 196608,
-#                "size": 65536 },
-#      "timestamp": { "seconds": 1378126126, "microseconds": 966463 } }
+#      "data": { "device": "", "node-name": "drive", "fatal": false,
+#                "msg": "L2 table offset 0x2a2a2a00 unaligned (L1 index: 0)" },
+#      "timestamp": { "seconds": 1648243240, "microseconds": 906060 } }
 #
 # Since: 1.7
 ##
-- 
2.35.1

Re: [PATCH v2 02/14] qapi: fix example of BLOCK_IMAGE_CORRUPTED event

[Victor Toso]

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

Hi,

On Mon, Mar 28, 2022 at 10:54:41AM +0200, Markus Armbruster wrote:
> Victor Toso <victortoso@redhat.com> writes:
> 
> > Fatal is not optional.
> 
> Recognizing that "Fatal" refers to member "fatal" takes mental
> effort.  Always use identifiers verbatim, including case.
> Where that makes for confusing prose, prefix with a @ like
> @fatal, or rephrase, like "The
> member fatal".

That's true.

> >
> > Signed-off-by: Victor Toso <victortoso@redhat.com>
> > ---
> >  qapi/block-core.json | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> >
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index e89f2dfb5b..63c30a5378 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -5006,10 +5006,9 @@
> >  # Example:
> >  #
> >  # <- { "event": "BLOCK_IMAGE_CORRUPTED",
> > -#      "data": { "device": "ide0-hd0", "node-name": "node0",
> > -#                "msg": "Prevented active L1 table overwrite", "offset": 196608,
> > -#                "size": 65536 },
> > -#      "timestamp": { "seconds": 1378126126, "microseconds": 966463 } }
> > +#      "data": { "device": "", "node-name": "drive", "fatal": false,
> > +#                "msg": "L2 table offset 0x2a2a2a00 unaligned (L1 index: 0)" },
> > +#      "timestamp": { "seconds": 1648243240, "microseconds": 906060 } }
> >  #
> >  # Since: 1.7
> >  ##
> 
> Changing the value of @msg makes sense, but is worth a note in the
> commit message.
> 
> Here's my try:
> 
>     qapi: fix example of BLOCK_IMAGE_CORRUPTED event
> 
>     Example output lacks mandatory member @fatal.  Provide it.
> 
>     Example output shows a value of @msg no version of the code
>     produces.  No big deal, but replace it anyway by one that today's
>     code does produce.
> 
> How do you like it?

I agree. I'll be more careful in near future. Thanks for the
suggestion. I've submit a v2.1 of this patch.

Cheers,
Victor

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2.1] qapi: fix example of BLOCK_IMAGE_CORRUPTED event

[Markus Armbruster]

Victor Toso <victortoso@redhat.com> writes:

> Example output lacks mandatory member @fatal.  Provide it.
>
> Example output shows a value of @msg no version of the code
> produces.  No big deal, but replace it anyway by one that
> today's code does produce.
>
> Signed-off-by: Victor Toso <victortoso@redhat.com>
> ---
>  qapi/block-core.json | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index e89f2dfb5b..63c30a5378 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -5006,10 +5006,9 @@
>  # Example:
>  #
>  # <- { "event": "BLOCK_IMAGE_CORRUPTED",
> -#      "data": { "device": "ide0-hd0", "node-name": "node0",
> -#                "msg": "Prevented active L1 table overwrite", "offset": 196608,
> -#                "size": 65536 },
> -#      "timestamp": { "seconds": 1378126126, "microseconds": 966463 } }
> +#      "data": { "device": "", "node-name": "drive", "fatal": false,
> +#                "msg": "L2 table offset 0x2a2a2a00 unaligned (L1 index: 0)" },
> +#      "timestamp": { "seconds": 1648243240, "microseconds": 906060 } }
>  #
>  # Since: 1.7
>  ##

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH v2 02/14] qapi: fix example of BLOCK_IMAGE_CORRUPTED event

[Markus Armbruster]

Victor Toso <victortoso@redhat.com> writes:

> Hi,
>
> On Mon, Mar 28, 2022 at 10:54:41AM +0200, Markus Armbruster wrote:
>> Victor Toso <victortoso@redhat.com> writes:
>> 
>> > Fatal is not optional.
>> 
>> Recognizing that "Fatal" refers to member "fatal" takes mental
>> effort.  Always use identifiers verbatim, including case.
>> Where that makes for confusing prose, prefix with a @ like
>> @fatal, or rephrase, like "The
>> member fatal".
>
> That's true.
>
>> >
>> > Signed-off-by: Victor Toso <victortoso@redhat.com>
>> > ---
>> >  qapi/block-core.json | 7 +++----
>> >  1 file changed, 3 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/qapi/block-core.json b/qapi/block-core.json
>> > index e89f2dfb5b..63c30a5378 100644
>> > --- a/qapi/block-core.json
>> > +++ b/qapi/block-core.json
>> > @@ -5006,10 +5006,9 @@
>> >  # Example:
>> >  #
>> >  # <- { "event": "BLOCK_IMAGE_CORRUPTED",
>> > -#      "data": { "device": "ide0-hd0", "node-name": "node0",
>> > -#                "msg": "Prevented active L1 table overwrite", "offset": 196608,
>> > -#                "size": 65536 },
>> > -#      "timestamp": { "seconds": 1378126126, "microseconds": 966463 } }
>> > +#      "data": { "device": "", "node-name": "drive", "fatal": false,
>> > +#                "msg": "L2 table offset 0x2a2a2a00 unaligned (L1 index: 0)" },
>> > +#      "timestamp": { "seconds": 1648243240, "microseconds": 906060 } }
>> >  #
>> >  # Since: 1.7
>> >  ##
>> 
>> Changing the value of @msg makes sense, but is worth a note in the
>> commit message.
>> 
>> Here's my try:
>> 
>>     qapi: fix example of BLOCK_IMAGE_CORRUPTED event
>> 
>>     Example output lacks mandatory member @fatal.  Provide it.
>> 
>>     Example output shows a value of @msg no version of the code
>>     produces.  No big deal, but replace it anyway by one that today's
>>     code does produce.
>> 
>> How do you like it?
>
> I agree. I'll be more careful in near future. Thanks for the
> suggestion. I've submit a v2.1 of this patch.

That's okay.  Asking me to replace the commit message in my tree is also
okay.

PATCH 3's commit message could use the same treatment.  Would you like
to go over all of them and respin?  Should be quick, no code changes.

Thanks!

Re: [PATCH v2 02/14] qapi: fix example of BLOCK_IMAGE_CORRUPTED event

[Victor Toso]

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

Hi,

On Mon, Mar 28, 2022 at 01:19:42PM +0200, Markus Armbruster wrote:
> Victor Toso <victortoso@redhat.com> writes:
> 
> > Hi,
> >
> > On Mon, Mar 28, 2022 at 10:54:41AM +0200, Markus Armbruster wrote:
> >> Victor Toso <victortoso@redhat.com> writes:
> >> 
> >> > Fatal is not optional.
> >> 
> >> Recognizing that "Fatal" refers to member "fatal" takes mental
> >> effort.  Always use identifiers verbatim, including case.
> >> Where that makes for confusing prose, prefix with a @ like
> >> @fatal, or rephrase, like "The
> >> member fatal".
> >
> > That's true.
> >
> >> >
> >> > Signed-off-by: Victor Toso <victortoso@redhat.com>
> >> > ---
> >> >  qapi/block-core.json | 7 +++----
> >> >  1 file changed, 3 insertions(+), 4 deletions(-)
> >> >
> >> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> >> > index e89f2dfb5b..63c30a5378 100644
> >> > --- a/qapi/block-core.json
> >> > +++ b/qapi/block-core.json
> >> > @@ -5006,10 +5006,9 @@
> >> >  # Example:
> >> >  #
> >> >  # <- { "event": "BLOCK_IMAGE_CORRUPTED",
> >> > -#      "data": { "device": "ide0-hd0", "node-name": "node0",
> >> > -#                "msg": "Prevented active L1 table overwrite", "offset": 196608,
> >> > -#                "size": 65536 },
> >> > -#      "timestamp": { "seconds": 1378126126, "microseconds": 966463 } }
> >> > +#      "data": { "device": "", "node-name": "drive", "fatal": false,
> >> > +#                "msg": "L2 table offset 0x2a2a2a00 unaligned (L1 index: 0)" },
> >> > +#      "timestamp": { "seconds": 1648243240, "microseconds": 906060 } }
> >> >  #
> >> >  # Since: 1.7
> >> >  ##
> >> 
> >> Changing the value of @msg makes sense, but is worth a note in the
> >> commit message.
> >> 
> >> Here's my try:
> >> 
> >>     qapi: fix example of BLOCK_IMAGE_CORRUPTED event
> >> 
> >>     Example output lacks mandatory member @fatal.  Provide it.
> >> 
> >>     Example output shows a value of @msg no version of the code
> >>     produces.  No big deal, but replace it anyway by one that today's
> >>     code does produce.
> >> 
> >> How do you like it?
> >
> > I agree. I'll be more careful in near future. Thanks for the
> > suggestion. I've submit a v2.1 of this patch.
> 
> That's okay.  Asking me to replace the commit message in my
> tree is also okay.
> 
> PATCH 3's commit message could use the same treatment.  Would
> you like to go over all of them and respin?  Should be quick,
> no code changes.

Sure, I'll be sending a v3 and amend the commit messages where
your previous suggestions fits.  I'll add the changes in the
cover-letter to help identify them.

Cheers,

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]