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

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

[Victor Toso]

Hi,

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.

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: run-state examples: add missing member
  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   |  9 +++++----
 qapi/block-export.json | 10 +++++-----
 qapi/dump.json         |  9 +++++----
 qapi/machine.json      |  3 ++-
 qapi/migration.json    |  4 +++-
 qapi/net.json          |  3 ++-
 qapi/run-state.json    | 16 +++++++++++-----
 qapi/ui.json           | 12 ++++++------
 9 files changed, 42 insertions(+), 29 deletions(-)

-- 
2.35.1

[PATCH 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>
---
 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 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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index e89f2dfb5b..585a9e020e 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -5008,7 +5008,7 @@
 # <- { "event": "BLOCK_IMAGE_CORRUPTED",
 #      "data": { "device": "ide0-hd0", "node-name": "node0",
 #                "msg": "Prevented active L1 table overwrite", "offset": 196608,
-#                "size": 65536 },
+#                "size": 65536, "fatal": false },
 #      "timestamp": { "seconds": 1378126126, "microseconds": 966463 } }
 #
 # Since: 1.7
-- 
2.35.1

[PATCH 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 585a9e020e..5b6c069dd9 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -5059,7 +5059,8 @@
 #      "data": { "device": "ide0-hd1",
 #                "node-name": "#block212",
 #                "operation": "write",
-#                "action": "stop" },
+#                "action": "stop",
+#                "reason": "Driver requires too large request alignment" },
 #      "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
 #
 ##
-- 
2.35.1

[PATCH 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>
---
 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 5b6c069dd9..ea96e1b009 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -5226,8 +5226,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 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"

Signed-off-by: Victor Toso <victortoso@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..d3ed79e8cd 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": 1265044230, "microseconds": 450486 } }
 #
 ##
 { 'event': 'DUMP_COMPLETED' ,
-- 
2.35.1

[PATCH 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>
---
 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 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 likes like most of examples do

Signed-off-by: Victor Toso <victortoso@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 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.

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

diff --git a/qapi/net.json b/qapi/net.json
index 7fab2e7cd8..82c0d9e778 100644
--- a/qapi/net.json
+++ b/qapi/net.json
@@ -691,7 +691,8 @@
 # Example:
 #
 # <- { "event": "FAILOVER_NEGOTIATED",
-#      "data": "net1" }
+#      "data": { "device-id": "net1" },
+#      "timestamp": { "seconds": 1368697518, "microseconds": 326866 } }
 #
 ##
 { 'event': 'FAILOVER_NEGOTIATED',
-- 
2.35.1

[PATCH 09/14] qapi: run-state examples: add missing member

[Victor Toso]

As reason member in not optional.

Signed-off-by: Victor Toso <victortoso@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 10/14] qapi: run-state examples: add missing timestamp

[Victor Toso]

Signed-off-by: Victor Toso <victortoso@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..f87b9378ac 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": 1267061043, "microseconds": 959568 } }
 #
 ##
 { 'event': 'GUEST_PANICKED',
@@ -446,7 +447,8 @@
 # Example:
 #
 # <- { "event": "GUEST_CRASHLOADED",
-#      "data": { "action": "run" } }
+#      "data": { "action": "run" },
+#      "timestamp": { "seconds": 1267061043, "microseconds": 959568 } }
 #
 ##
 { 'event': 'GUEST_CRASHLOADED',
-- 
2.35.1

[PATCH 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>
---
 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 f87b9378ac..ee21decdec 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 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>
---
 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 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>
---
 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 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>
---
 qapi/dump.json | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qapi/dump.json b/qapi/dump.json
index d3ed79e8cd..0873f16e5c 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 02/14] qapi: fix example of BLOCK_IMAGE_CORRUPTED event

[John Snow]

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

On Thu, Mar 24, 2022, 1:50 PM Victor Toso <victortoso@redhat.com> wrote:

> Fatal is not optional.
>
> Signed-off-by: Victor Toso <victortoso@redhat.com>
> ---
>  qapi/block-core.json | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index e89f2dfb5b..585a9e020e 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -5008,7 +5008,7 @@
>  # <- { "event": "BLOCK_IMAGE_CORRUPTED",
>  #      "data": { "device": "ide0-hd0", "node-name": "node0",
>  #                "msg": "Prevented active L1 table overwrite", "offset":
> 196608,
> -#                "size": 65536 },
> +#                "size": 65536, "fatal": false },
>  #      "timestamp": { "seconds": 1378126126, "microseconds": 966463 } }
>  #
>  # Since: 1.7
> --
> 2.35.1
>

Is this the correct fatality setting for this particular case? Default is
implied to be true.

>

[-- Attachment #2: Type: text/html, Size: 1754 bytes --]

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

[John Snow]

On Thu, Mar 24, 2022 at 1:50 PM Victor Toso <victortoso@redhat.com> wrote:
>
> @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>
> ---
>  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
>

Does this help with something in particular? (Got an example for me?)

--js

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

[John Snow]

On Thu, Mar 24, 2022 at 3:15 PM John Snow <jsnow@redhat.com> wrote:
>
>
>
> On Thu, Mar 24, 2022, 1:50 PM Victor Toso <victortoso@redhat.com> wrote:
>>
>> Fatal is not optional.
>>
>> Signed-off-by: Victor Toso <victortoso@redhat.com>
>> ---
>>  qapi/block-core.json | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index e89f2dfb5b..585a9e020e 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -5008,7 +5008,7 @@
>>  # <- { "event": "BLOCK_IMAGE_CORRUPTED",
>>  #      "data": { "device": "ide0-hd0", "node-name": "node0",
>>  #                "msg": "Prevented active L1 table overwrite", "offset": 196608,
>> -#                "size": 65536 },
>> +#                "size": 65536, "fatal": false },
>>  #      "timestamp": { "seconds": 1378126126, "microseconds": 966463 } }
>>  #
>>  # Since: 1.7
>> --
>> 2.35.1
>
>
> Is this the correct fatality setting for this particular case? Default is implied to be true.

(1) We don't seem to actually emit this particular message anymore. I
don't think it exists in the tree.

(2) The only fatal=False messages I can see is
"Cannot free unaligned cluster %#llx"

(Try grepping for qcow2_signal_corruption)

so maybe we should pick a new example that might really exist. iotest
060 seems to test this, so that can be used as a guide.

--js

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

[John Snow]

On Thu, Mar 24, 2022 at 1:50 PM Victor Toso <victortoso@redhat.com> wrote:
>
> 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 585a9e020e..5b6c069dd9 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -5059,7 +5059,8 @@
>  #      "data": { "device": "ide0-hd1",
>  #                "node-name": "#block212",
>  #                "operation": "write",
> -#                "action": "stop" },
> +#                "action": "stop",
> +#                "reason": "Driver requires too large request alignment" },
>  #      "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
>  #
>  ##
> --
> 2.35.1
>

We discourage people using the reason programmatically, but there will
indeed be one. Where'd you pull the message out from?

I see this:

static void send_qmp_error_event(BlockBackend *blk,
                                 BlockErrorAction action,
                                 bool is_read, int error)
{
    IoOperationType optype;
    BlockDriverState *bs = blk_bs(blk);

    optype = is_read ? IO_OPERATION_TYPE_READ : IO_OPERATION_TYPE_WRITE;
    qapi_event_send_block_io_error(blk_name(blk), !!bs,
                                   bs ? bdrv_get_node_name(bs) : NULL, optype,
                                   action, blk_iostatus_is_enabled(blk),
                                   error == ENOSPC, strerror(error));
}


so it should be one of the "standard" strerror messages, right?

--js

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

[John Snow]

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

On Thu, Mar 24, 2022, 1:50 PM Victor Toso <victortoso@redhat.com> wrote:

> * Event's name: BLOCK_JOB_WAITING -> BLOCK_JOB_PENDING
> * Argument device -> id
>
> Signed-off-by: Victor Toso <victortoso@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 5b6c069dd9..ea96e1b009 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -5226,8 +5226,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
>

Ow, how'd I get away with this? It was just always wrong and we never
noticed?

Cool.

Reviewed-by: John Snow <jsnow@redhat.com>

(Whoa, this is from 2018? It feels like it was from way before then.)

>

[-- Attachment #2: Type: text/html, Size: 2013 bytes --]

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

[John Snow]

On Thu, Mar 24, 2022 at 1:50 PM Victor Toso <victortoso@redhat.com> wrote:
>
> * 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"
>
> Signed-off-by: Victor Toso <victortoso@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..d3ed79e8cd 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": 1265044230, "microseconds": 450486 } }
>  #
>  ##
>  { 'event': 'DUMP_COMPLETED' ,
> --
> 2.35.1
>

Other events seem to use the timestamp as well, so go for it. I agree
that being able to programmatically verify docstrings is pretty
valuable in an API test suite.

(What date did you choose? Does it mean anything to you? :p)

Reviewed-by: John Snow <jsnow@redhat.com>

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

[John Snow]

On Thu, Mar 24, 2022 at 1:50 PM Victor Toso <victortoso@redhat.com> wrote:
>
> * qom-path is not optional
>
> Signed-off-by: Victor Toso <victortoso@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
>

I'll just assume this is a realistic qom-path and not actually try to check 😅

Reviewed-by: John Snow <jsnow@redhat.com>

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

[John Snow]

On Thu, Mar 24, 2022 at 1:50 PM Victor Toso <victortoso@redhat.com> wrote:
>
> * Timestamp is not optional, let's add for completeness.
> * Add '<-' to signalize it is receiving the data
> * Break likes like most of examples do

Oh, I think you meant "break lines like". That took a long minute to parse.

>
> Signed-off-by: Victor Toso <victortoso@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
>

With commit message amended:

Reviewed-by: John Snow <jsnow@redhat.com>

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

[John Snow]

On Thu, Mar 24, 2022 at 1:50 PM Victor Toso <victortoso@redhat.com> wrote:
>
> * Data is an object, not a string. It generates a qdict.
> * Timestamp is not optional, let's add for completeness.
>
> Signed-off-by: Victor Toso <victortoso@redhat.com>
> ---
>  qapi/net.json | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/qapi/net.json b/qapi/net.json
> index 7fab2e7cd8..82c0d9e778 100644
> --- a/qapi/net.json
> +++ b/qapi/net.json
> @@ -691,7 +691,8 @@
>  # Example:
>  #
>  # <- { "event": "FAILOVER_NEGOTIATED",
> -#      "data": "net1" }
> +#      "data": { "device-id": "net1" },
> +#      "timestamp": { "seconds": 1368697518, "microseconds": 326866 } }
>  #
>  ##
>  { 'event': 'FAILOVER_NEGOTIATED',
> --
> 2.35.1
>

Oh, this one is all messed up to hell. We're not documenting the
device-id properly, either. Can that be fixed as well?

Your patch ain't wrong, though:

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [PATCH 09/14] qapi: run-state examples: add missing member

[John Snow]

On Thu, Mar 24, 2022 at 1:50 PM Victor Toso <victortoso@redhat.com> wrote:
>
> As reason member in not optional.

Suggest:

"The 'reason' member is not optional."

I also like how you included the type/structure name in the other
commit messages, can you work "SHUTDOWN" into this one?

>
> Signed-off-by: Victor Toso <victortoso@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
>

With commit tweaks:

Reviewed-by: John Snow <jsnow@redhat.com>

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

[John Snow]

On Thu, Mar 24, 2022 at 1:50 PM Victor Toso <victortoso@redhat.com> wrote:
>
> Signed-off-by: Victor Toso <victortoso@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..f87b9378ac 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": 1267061043, "microseconds": 959568 } }
>  #
>  ##
>  { 'event': 'GUEST_PANICKED',
> @@ -446,7 +447,8 @@
>  # Example:
>  #
>  # <- { "event": "GUEST_CRASHLOADED",
> -#      "data": { "action": "run" } }
> +#      "data": { "action": "run" },
> +#      "timestamp": { "seconds": 1267061043, "microseconds": 959568 } }
>  #
>  ##
>  { 'event': 'GUEST_CRASHLOADED',
> --
> 2.35.1
>

Someone once reviewed my documentation and noted that the timestamps
were correctly chronological.

... I feel like I have been *hurt* somehow.

Anyway:

Reviewed-by: John Snow <jsnow@redhat.com>

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

[John Snow]

On Thu, Mar 24, 2022 at 1:50 PM Victor Toso <victortoso@redhat.com> wrote:
>
> As the websocket is not optional in VncBasicInfo.
>
> Signed-off-by: Victor Toso <victortoso@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
>

Okie-dokey.

Reviewed-by: John Snow <jsnow@redhat.com>

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

[John Snow]

On Thu, Mar 24, 2022 at 1:50 PM Victor Toso <victortoso@redhat.com> wrote:
>
> 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.

Oh, yeah. Good call.

> * Changed string from '' to "" in action-required member.
>
> Signed-off-by: Victor Toso <victortoso@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 f87b9378ac..ee21decdec 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
>

Reviewed-by: John Snow <jsnow@redhat.com>

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

[John Snow]

On Thu, Mar 24, 2022 at 1:50 PM Victor Toso <victortoso@redhat.com> wrote:
>
> * Missing the timestamp
> * Missing the "info" object in the "data" member
>
> Signed-off-by: Victor Toso <victortoso@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
>

Oh, good spot.

Reviewed-by: John Snow <jsnow@redhat.com>

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

[John Snow]

On Thu, Mar 24, 2022 at 1:50 PM Victor Toso <victortoso@redhat.com> wrote:
>
> The member "paging" is not optional
>
> Signed-off-by: Victor Toso <victortoso@redhat.com>
> ---
>  qapi/dump.json | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/qapi/dump.json b/qapi/dump.json
> index d3ed79e8cd..0873f16e5c 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
>

Assuming this is a feasible example.

Reviewed-by: John Snow <jsnow@redhat.com>

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

[John Snow]

On Thu, Mar 24, 2022 at 1:50 PM Victor Toso <victortoso@redhat.com> wrote:
>
> Hi,
>
> 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.
>
> 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: run-state examples: add missing member
>   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   |  9 +++++----
>  qapi/block-export.json | 10 +++++-----
>  qapi/dump.json         |  9 +++++----
>  qapi/machine.json      |  3 ++-
>  qapi/migration.json    |  4 +++-
>  qapi/net.json          |  3 ++-
>  qapi/run-state.json    | 16 +++++++++++-----
>  qapi/ui.json           | 12 ++++++------
>  9 files changed, 42 insertions(+), 29 deletions(-)
>
> --
> 2.35.1
>

Good stuff, IMO. Systematically validating our docs will be a good
thing for improving the usability of those docs.

Only the first patch seems like a lateral move, but I assume you had
your reasons.

--js

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

[Markus Armbruster]

Victor Toso <victortoso@redhat.com> writes:

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

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

Doc comments embed user documentation in the source code.  The doc
generator extracts it.

TODOs are generally for developers.  Should the doc generator suppress
TODO sections?

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

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On Thu, Mar 24, 2022 at 3:15 PM John Snow <jsnow@redhat.com> wrote:
>>
>>
>>
>> On Thu, Mar 24, 2022, 1:50 PM Victor Toso <victortoso@redhat.com> wrote:
>>>
>>> Fatal is not optional.
>>>
>>> Signed-off-by: Victor Toso <victortoso@redhat.com>
>>> ---
>>>  qapi/block-core.json | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>> index e89f2dfb5b..585a9e020e 100644
>>> --- a/qapi/block-core.json
>>> +++ b/qapi/block-core.json
>>> @@ -5008,7 +5008,7 @@
>>>  # <- { "event": "BLOCK_IMAGE_CORRUPTED",
>>>  #      "data": { "device": "ide0-hd0", "node-name": "node0",
>>>  #                "msg": "Prevented active L1 table overwrite", "offset": 196608,
>>> -#                "size": 65536 },
>>> +#                "size": 65536, "fatal": false },
>>>  #      "timestamp": { "seconds": 1378126126, "microseconds": 966463 } }
>>>  #
>>>  # Since: 1.7
>>> --
>>> 2.35.1
>>
>>
>> Is this the correct fatality setting for this particular case? Default is implied to be true.
>
> (1) We don't seem to actually emit this particular message anymore. I
> don't think it exists in the tree.

I doubt we ever emitted it.

> (2) The only fatal=False messages I can see is
> "Cannot free unaligned cluster %#llx"
>
> (Try grepping for qcow2_signal_corruption)
>
> so maybe we should pick a new example that might really exist. iotest
> 060 seems to test this, so that can be used as a guide.

Yes, please.

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

[Markus Armbruster]

Victor Toso <victortoso@redhat.com> writes:

> * Event's name: BLOCK_JOB_WAITING -> BLOCK_JOB_PENDING
> * Argument device -> id
>
> Signed-off-by: Victor Toso <victortoso@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 5b6c069dd9..ea96e1b009 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -5226,8 +5226,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 } }
>  #
>  ##

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

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

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On Thu, Mar 24, 2022 at 1:50 PM Victor Toso <victortoso@redhat.com> wrote:
>>
>> * 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"
>>
>> Signed-off-by: Victor Toso <victortoso@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..d3ed79e8cd 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} },

Add a space after 1090650112, too?

Aside: I don't actually like our use of spaces in JSON, but consistently
ugly beats inconsistently ugly.

>> +#      "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
>>  #
>>  ##
>>  { 'event': 'DUMP_COMPLETED' ,
>> --
>> 2.35.1
>>
>
> Other events seem to use the timestamp as well, so go for it. I agree
> that being able to programmatically verify docstrings is pretty
> valuable in an API test suite.
>
> (What date did you choose? Does it mean anything to you? :p)

Copied from some other example, I suppose.  I'd probably use time of
writing, but that's just me.

> Reviewed-by: John Snow <jsnow@redhat.com>

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

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

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On Thu, Mar 24, 2022 at 1:50 PM Victor Toso <victortoso@redhat.com> wrote:
>>
>> * qom-path is not optional

List of one item.  Recommend to scratch '* '.  Slightly less terse, like
"Event data member @qom-path is not optional" wouldn't hurt.

>>
>> Signed-off-by: Victor Toso <victortoso@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
>>
>
> I'll just assume this is a realistic qom-path and not actually try to check 😅

I suppose a machine could exist where this path leads to a suitable
device.

> Reviewed-by: John Snow <jsnow@redhat.com>

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

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

[John Snow]

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

On Fri, Mar 25, 2022, 8:33 AM Markus Armbruster <armbru@redhat.com> wrote:

> Victor Toso <victortoso@redhat.com> writes:
>
> > @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>
> > ---
> >  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
> >  ##
>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>
> Doc comments embed user documentation in the source code.  The doc
> generator extracts it.
>
> TODOs are generally for developers.  Should the doc generator suppress
> TODO sections?
>

Needs an audit to make sure we're using it consistently with that semantic,
but broadly it's probably a good idea to squelch "internal" todos, yes.

Things like "Watch out, were definitely gonna deprecate this soon probably
maybe!" can stay outside of the TODO section. (Sometimes heads up are
legitimate, even if most won't read them. the faithful and diligent will be
rewarded with painless upgrades.)

Anyway, if Markus is happy with this change, I am too, I was just curious
to know if there were bigger cleanups to do here and what the impact was.

Anyway:

Reviewed-by: John Snow <jsnow@redhat.com>

>

[-- Attachment #2: Type: text/html, Size: 3602 bytes --]

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

[Victor Toso]

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

Hi,

Many thanks for the quick review!

On Thu, Mar 24, 2022 at 04:34:42PM -0400, John Snow wrote:
> On Thu, Mar 24, 2022 at 1:50 PM Victor Toso <victortoso@redhat.com> wrote:
> >
> > @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>
> > ---
> >  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
> >
> 
> Does this help with something in particular? (Got an example for me?)

I'm working on a Golang interface and I'm using the QAPI
documentation to document the Go's types of the QAPI spec. For this
kind of documentation, documentation related to future development
can be excluded. This patch helps me filter it out :)

Example:

  $ cd qemu/scripts && python
  >>> from qapi.schema import QAPISchema
  >>> schema = QAPISchema('../qapi/qapi-schema.json')

  # Without this patch, the 'Potential additional modes' doc is
  # under no specific named Section.
  >>> for s in schema._entity_dict['BlockExportRemoveMode'].doc.sections:
  ...     pprint(vars(s))
  ...
  {'_indent': 0,
   '_parser': <qapi.parser.QAPISchemaParser object at 0x7f4fcf854760>,
   'name': None,
   'text': 'Potential additional modes to be added in the future:\n'
           '\n'
           'hide: Just hide export from new clients, leave existing connections '
           'as is.\n'
           'Remove export after all clients are disconnected.\n'
           '\n'
           'soft: Hide export from new clients, answer with ESHUTDOWN for all '
           'further\n'
           'requests from existing clients.'}
   {'_indent': 7,
   '_parser': <qapi.parser.QAPISchemaParser object at 0x7f4fcf854760>,
   'name': 'Since',
   'text': '2.12'}

  # With this patch, we can filter out TODO section
  >>> pprint(schema._entity_dict['BlockExportRemoveMode'].doc.sections[0]))
  {'_indent': 6,
   '_parser': <qapi.parser.QAPISchemaParser object at 0x7f228d97e950>,
   'name': 'TODO',
   'text': 'Potential additional modes to be added in the future:\n'
           '\n'
           'hide: Just hide export from new clients, leave existing connections '
           'as is.\n'
           'Remove export after all clients are disconnected.\n'
           '\n'
           'soft: Hide export from new clients, answer with ESHUTDOWN for all '
           'further\n'
           'requests from existing clients.'}

Cheers,
Victor

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

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

[Victor Toso]

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

Hi,

On Fri, Mar 25, 2022 at 11:11:23AM -0400, John Snow wrote:
> On Fri, Mar 25, 2022, 8:33 AM Markus Armbruster <armbru@redhat.com> wrote:
> 
> > Victor Toso <victortoso@redhat.com> writes:
> >
> > > @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>
> > > ---
> > >  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
> > >  ##
> >
> > Reviewed-by: Markus Armbruster <armbru@redhat.com>

Thanks,

> > Doc comments embed user documentation in the source code.  The doc
> > generator extracts it.
> >
> > TODOs are generally for developers.  Should the doc generator suppress
> > TODO sections?
> 
> Needs an audit to make sure we're using it consistently with
> that semantic, but broadly it's probably a good idea to squelch
> "internal" todos, yes.
> 
> Things like "Watch out, were definitely gonna deprecate this
> soon probably maybe!" can stay outside of the TODO section.
> (Sometimes heads up are legitimate, even if most won't read
> them. the faithful and diligent will be rewarded with painless
> upgrades.)

There are 5 TODO sections in QAPI (including this patch):

 qapi/block-export.json:222:# TODO: Potential additional modes to be added in the future:
 qapi/introspect.json:300:# TODO: @success-response (currently irrelevant, because it's QGA, not QMP)
 qapi/machine.json:913:# TODO: Better documentation; currently there is none.
 qapi/migration.json:933:# TODO either fuse back into MigrationParameters, or make
 qapi/qdev.json:70:# TODO: This command effectively bypasses QAPI completely due to its

I think their usage is a bit broad but helpful.

> Anyway, if Markus is happy with this change, I am too, I was
> just curious to know if there were bigger cleanups to do here
> and what the impact was.

I'll let you know if I find more :)

> Anyway:
> 
> Reviewed-by: John Snow <jsnow@redhat.com>

Thanks! I'll send a v2 later with all suggestions.

Cheers,
Victor

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

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

[Victor Toso]

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

Hi,

On Thu, Mar 24, 2022 at 04:47:30PM -0400, John Snow wrote:
> On Thu, Mar 24, 2022 at 1:50 PM Victor Toso <victortoso@redhat.com> wrote:
> >
> > 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 585a9e020e..5b6c069dd9 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -5059,7 +5059,8 @@
> >  #      "data": { "device": "ide0-hd1",
> >  #                "node-name": "#block212",
> >  #                "operation": "write",
> > -#                "action": "stop" },
> > +#                "action": "stop",
> > +#                "reason": "Driver requires too large request alignment" },
> >  #      "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
> >  #
> >  ##
> > --
> > 2.35.1
> >
> 
> We discourage people using the reason programmatically, but
> there will indeed be one. Where'd you pull the message out
> from?

I was looking into the block related errors and pick a string. It
wasn't a real error that I had.

> I see this:
> 
> static void send_qmp_error_event(BlockBackend *blk,
>                                  BlockErrorAction action,
>                                  bool is_read, int error)
> {
>     IoOperationType optype;
>     BlockDriverState *bs = blk_bs(blk);
> 
>     optype = is_read ? IO_OPERATION_TYPE_READ : IO_OPERATION_TYPE_WRITE;
>     qapi_event_send_block_io_error(blk_name(blk), !!bs,
>                                    bs ? bdrv_get_node_name(bs) : NULL, optype,
>                                    action, blk_iostatus_is_enabled(blk),
>                                    error == ENOSPC, strerror(error));
> }
> 
> 
> so it should be one of the "standard" strerror messages, right?

Yep. I'll pick that one and use 'No space left on device'

Cheers,

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

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

[Victor Toso]

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

Hi,

On Fri, Mar 25, 2022 at 01:43:06PM +0100, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
> > On Thu, Mar 24, 2022 at 3:15 PM John Snow <jsnow@redhat.com> wrote:
> >>
> >>
> >>
> >> On Thu, Mar 24, 2022, 1:50 PM Victor Toso <victortoso@redhat.com> wrote:
> >>>
> >>> Fatal is not optional.
> >>>
> >>> Signed-off-by: Victor Toso <victortoso@redhat.com>
> >>> ---
> >>>  qapi/block-core.json | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/qapi/block-core.json b/qapi/block-core.json
> >>> index e89f2dfb5b..585a9e020e 100644
> >>> --- a/qapi/block-core.json
> >>> +++ b/qapi/block-core.json
> >>> @@ -5008,7 +5008,7 @@
> >>>  # <- { "event": "BLOCK_IMAGE_CORRUPTED",
> >>>  #      "data": { "device": "ide0-hd0", "node-name": "node0",
> >>>  #                "msg": "Prevented active L1 table overwrite", "offset": 196608,
> >>> -#                "size": 65536 },
> >>> +#                "size": 65536, "fatal": false },
> >>>  #      "timestamp": { "seconds": 1378126126, "microseconds": 966463 } }
> >>>  #
> >>>  # Since: 1.7
> >>> --
> >>> 2.35.1
> >>
> >>
> >> Is this the correct fatality setting for this particular case? Default is implied to be true.
> >
> > (1) We don't seem to actually emit this particular message anymore. I
> > don't think it exists in the tree.
> 
> I doubt we ever emitted it.

Out of curiosity, shouldn't we deprecate it then?

> > (2) The only fatal=False messages I can see is
> > "Cannot free unaligned cluster %#llx"
> >
> > (Try grepping for qcow2_signal_corruption)
> >
> > so maybe we should pick a new example that might really exist. iotest
> > 060 seems to test this, so that can be used as a guide.
> 
> Yes, please.

I'll be changing it on v2

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

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

[Victor Toso]

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

Hi,

On Fri, Mar 25, 2022 at 01:53:50PM +0100, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
> > On Thu, Mar 24, 2022 at 1:50 PM Victor Toso <victortoso@redhat.com> wrote:
> >>
> >> * 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"
> >>
> >> Signed-off-by: Victor Toso <victortoso@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..d3ed79e8cd 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} },
> 
> Add a space after 1090650112, too?

ok!
 
> Aside: I don't actually like our use of spaces in JSON, but consistently
> ugly beats inconsistently ugly.

Ideally it would be better use some tool to pretty format/sort it
so we don't even bother looking at it too much.
 
> >> +#      "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
> >>  #
> >>  ##
> >>  { 'event': 'DUMP_COMPLETED' ,
> >> --
> >> 2.35.1
> >>
> >
> > Other events seem to use the timestamp as well, so go for it. I agree
> > that being able to programmatically verify docstrings is pretty
> > valuable in an API test suite.
> >
> > (What date did you choose? Does it mean anything to you? :p)
> 
> Copied from some other example, I suppose.

Yes,
> I'd probably use time of writing, but that's just me.

Never thought about it. Why not, I'll change it.

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

Thanks!

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

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

[Victor Toso]

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

On Thu, Mar 24, 2022 at 04:57:00PM -0400, John Snow wrote:
> On Thu, Mar 24, 2022 at 1:50 PM Victor Toso <victortoso@redhat.com> wrote:
> >
> > * qom-path is not optional
> >
> > Signed-off-by: Victor Toso <victortoso@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
> >
> 
> I'll just assume this is a realistic qom-path and not actually try to check 😅

Copied from another example, so blame that one if it doesn't!

> Reviewed-by: John Snow <jsnow@redhat.com>

Cheers,

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

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

[John Snow]

On Fri, Mar 25, 2022 at 4:59 PM Victor Toso <victortoso@redhat.com> wrote:
>
> Hi,
>
> On Fri, Mar 25, 2022 at 01:43:06PM +0100, Markus Armbruster wrote:
> > John Snow <jsnow@redhat.com> writes:
> >
> > > On Thu, Mar 24, 2022 at 3:15 PM John Snow <jsnow@redhat.com> wrote:
> > >>
> > >>
> > >>
> > >> On Thu, Mar 24, 2022, 1:50 PM Victor Toso <victortoso@redhat.com> wrote:
> > >>>
> > >>> Fatal is not optional.
> > >>>
> > >>> Signed-off-by: Victor Toso <victortoso@redhat.com>
> > >>> ---
> > >>>  qapi/block-core.json | 2 +-
> > >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> > >>>
> > >>> diff --git a/qapi/block-core.json b/qapi/block-core.json
> > >>> index e89f2dfb5b..585a9e020e 100644
> > >>> --- a/qapi/block-core.json
> > >>> +++ b/qapi/block-core.json
> > >>> @@ -5008,7 +5008,7 @@
> > >>>  # <- { "event": "BLOCK_IMAGE_CORRUPTED",
> > >>>  #      "data": { "device": "ide0-hd0", "node-name": "node0",
> > >>>  #                "msg": "Prevented active L1 table overwrite", "offset": 196608,
> > >>> -#                "size": 65536 },
> > >>> +#                "size": 65536, "fatal": false },
> > >>>  #      "timestamp": { "seconds": 1378126126, "microseconds": 966463 } }
> > >>>  #
> > >>>  # Since: 1.7
> > >>> --
> > >>> 2.35.1
> > >>
> > >>
> > >> Is this the correct fatality setting for this particular case? Default is implied to be true.
> > >
> > > (1) We don't seem to actually emit this particular message anymore. I
> > > don't think it exists in the tree.
> >
> > I doubt we ever emitted it.
>
> Out of curiosity, shouldn't we deprecate it then?
>

He means: we probably never emitted that specific error message. The
*event* is in use, see iotest 060.

> > > (2) The only fatal=False messages I can see is
> > > "Cannot free unaligned cluster %#llx"
> > >
> > > (Try grepping for qcow2_signal_corruption)
> > >
> > > so maybe we should pick a new example that might really exist. iotest
> > > 060 seems to test this, so that can be used as a guide.
> >
> > Yes, please.
>
> I'll be changing it on v2

"Future directions" vs. "TODO" in doc comments (was: [PATCH 01/14] qapi: BlockExportRemoveMode: move comments to TODO)

[Markus Armbruster]

Victor Toso <victortoso@redhat.com> writes:

> Hi,
>
> On Fri, Mar 25, 2022 at 11:11:23AM -0400, John Snow wrote:
>> On Fri, Mar 25, 2022, 8:33 AM Markus Armbruster <armbru@redhat.com> wrote:

[...]

>> > Doc comments embed user documentation in the source code.  The doc
>> > generator extracts it.
>> >
>> > TODOs are generally for developers.  Should the doc generator suppress
>> > TODO sections?
>> 
>> Needs an audit to make sure we're using it consistently with
>> that semantic, but broadly it's probably a good idea to squelch
>> "internal" todos, yes.
>> 
>> Things like "Watch out, were definitely gonna deprecate this
>> soon probably maybe!" can stay outside of the TODO section.
>> (Sometimes heads up are legitimate, even if most won't read
>> them. the faithful and diligent will be rewarded with painless
>> upgrades.)

This is "future directions", not quite the same as "TODO".

Would a section tag "Future directions" make sense?

> There are 5 TODO sections in QAPI (including this patch):

Let me try to sort them into "TODO" and "future directions" buckets.
The former are of interest for developers only, and thus should be
elided from documentation meant for users.

>  qapi/block-export.json:222:# TODO: Potential additional modes to be added in the future:

Do we believe our thoughts on evolving of this enum are relevant for
users of the affected QMP commands (nbd-server-remove and
block-export-del)?

If yes, it's "future directions".

>  qapi/introspect.json:300:# TODO: @success-response (currently irrelevant, because it's QGA, not QMP)

As phrased, this is only useful for developers, and even for them, it's
rather terse.

If we add introspection to QGA, we'll want to add a @success-response
member.

So, if we intend to add introspection to QGA, *and* we think current
users of (QMP-only) introspection need to know about a future addition
of @success-response, then this should be rephrased as "future
directions".

I doubt it.

>  qapi/machine.json:913:# TODO: Better documentation; currently there is none.

Clearly TODO.

>  qapi/migration.json:933:# TODO either fuse back into MigrationParameters, or make

Clearly TODO.  Note that this one is *not* in a doc comment, and does
*not* appear in generated documentation.

Once we have concrete plans on how to address the TODO, these plans may
motivate "future directions", namely if they involve user-visible change
users need to know about in advance.

>  qapi/qdev.json:70:# TODO: This command effectively bypasses QAPI completely due to its

Likewise.

I think this shows that we have a few comments just for developers in
the middle of user documentation.

We could simply keep these outside doc comments, like the TODO in
qapi/migration.json.

This can occasionally be awkward.  For instance, TODO @success-response
is right where @success-response ought to be.  Moving it outside the doc
comment would lose that.  Not the end of the world, just awkward.

If this annoys us enough, we could provide means to let us have elide
parts of doc comments from generated docs.  The simplest one is probably
eliding certain sections, say the TODO sections.

Thoughts?

[...]

Re: "Future directions" vs. "TODO" in doc comments (was: [PATCH 01/14] qapi: BlockExportRemoveMode: move comments to TODO)

[John Snow]

On Mon, Mar 28, 2022 at 3:46 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> Victor Toso <victortoso@redhat.com> writes:
>
> > Hi,
> >
> > On Fri, Mar 25, 2022 at 11:11:23AM -0400, John Snow wrote:
> >> On Fri, Mar 25, 2022, 8:33 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> [...]
>
> >> > Doc comments embed user documentation in the source code.  The doc
> >> > generator extracts it.
> >> >
> >> > TODOs are generally for developers.  Should the doc generator suppress
> >> > TODO sections?
> >>
> >> Needs an audit to make sure we're using it consistently with
> >> that semantic, but broadly it's probably a good idea to squelch
> >> "internal" todos, yes.
> >>
> >> Things like "Watch out, were definitely gonna deprecate this
> >> soon probably maybe!" can stay outside of the TODO section.
> >> (Sometimes heads up are legitimate, even if most won't read
> >> them. the faithful and diligent will be rewarded with painless
> >> upgrades.)
>
> This is "future directions", not quite the same as "TODO".
>
> Would a section tag "Future directions" make sense?
>

If we're leaving these kinds of warnings already, sure. I was just
speaking generically.
I didn't actually audit all of our docs to see what types of notes
we've left so far.

> > There are 5 TODO sections in QAPI (including this patch):
>
> Let me try to sort them into "TODO" and "future directions" buckets.
> The former are of interest for developers only, and thus should be
> elided from documentation meant for users.
>

Oh, but it looks like *you* did the audit ;)

> >  qapi/block-export.json:222:# TODO: Potential additional modes to be added in the future:
>
> Do we believe our thoughts on evolving of this enum are relevant for
> users of the affected QMP commands (nbd-server-remove and
> block-export-del)?
>
> If yes, it's "future directions".
>

Presumably this evolves compatibly like QMP always does, and doesn't
necessarily warrant a special note. "future directions" feels
reasonable.

> >  qapi/introspect.json:300:# TODO: @success-response (currently irrelevant, because it's QGA, not QMP)
>
> As phrased, this is only useful for developers, and even for them, it's
> rather terse.
>
> If we add introspection to QGA, we'll want to add a @success-response
> member.
>
> So, if we intend to add introspection to QGA, *and* we think current
> users of (QMP-only) introspection need to know about a future addition
> of @success-response, then this should be rephrased as "future
> directions".
>
> I doubt it.
>

I'll defer to your judgment on this one. The last time I tried to bark
into QGA I got more than I bargained for. :')

> >  qapi/machine.json:913:# TODO: Better documentation; currently there is none.
>
> Clearly TODO.
>

Yuh

> >  qapi/migration.json:933:# TODO either fuse back into MigrationParameters, or make
>
> Clearly TODO.  Note that this one is *not* in a doc comment, and does
> *not* appear in generated documentation.
>
> Once we have concrete plans on how to address the TODO, these plans may
> motivate "future directions", namely if they involve user-visible change
> users need to know about in advance.
>
> >  qapi/qdev.json:70:# TODO: This command effectively bypasses QAPI completely due to its
>
> Likewise.
>
> I think this shows that we have a few comments just for developers in
> the middle of user documentation.
>
> We could simply keep these outside doc comments, like the TODO in
> qapi/migration.json.
>
> This can occasionally be awkward.  For instance, TODO @success-response
> is right where @success-response ought to be.  Moving it outside the doc
> comment would lose that.  Not the end of the world, just awkward.
>
> If this annoys us enough, we could provide means to let us have elide
> parts of doc comments from generated docs.  The simplest one is probably
> eliding certain sections, say the TODO sections.
>
> Thoughts?
>

I'm fine with it, maybe pending the spelling of such a section.
Keeping the docs clean and useful on generation sounds like a noble
endeavor. I don't think it's urgent, but I'm willing to let Victor
tell me which pieces are annoying and getting in his way.

(I still want to redo our docstring handling, but it wound up being a
bigger project than I could bite off in a week or two. It's extremely
high on my list of things to do, but I am already in debt to you on
the typing project I wanted to engage on, so ... I will sit on my
hands for right now.)