[PATCH v1 00/16] qapi examples fixes and rfc for another generator

[PATCH v1 00/16] qapi examples fixes and rfc for another generator

[Victor Toso]

Hi,

I've been using the examples as a way to test the qapi-go proposal [0]
and I thought it would be neat to have a way to parse those examples and
regenerate them in a way that we could use for testing.

The first 10 patches are fixes to issues found when loading the Example
into python's json library.

Patches 11-15 are fixes to issues found when using those exampels in the
qapi-go module [1].

Patch 16 is the RFC for the generator used. If the idea is interesting,
I can rework it and add some qapi/qmp verification of the examples at
generation time with python-qemu-qmp [2].

[0] https://lists.nongnu.org/archive/html/qemu-devel/2022-06/msg03105.html
[1] https://gitlab.com/victortoso/qapi-go/-/tree/wip-v3/test
[2] https://gitlab.com/qemu-project/python-qemu-qmp

Cheers,
Victor

Victor Toso (16):
  qapi: fix example of query-ballon command
  qapi: fix example of query-vnc command
  qapi: fix example of query-spice command
  qapi: fix example of query-rocker-of-dpa-flows command
  qapi: fix example of query-dump-guest-memory-capability command
  qapi: fix example of query-blockstats command
  qapi: fix example of BLOCK_JOB_READY event
  qapi: fix example of NIC_RX_FILTER_CHANGED event
  qapi: fix example of DEVICE_UNPLUG_GUEST_ERROR event
  qapi: fix example of MEM_UNPLUG_ERROR event
  qapi: fix examples of blockdev-add with qcow2
  qapi: fix example of blockdev-add command
  qapi: fix example of query-hotpluggable-cpus command
  qapi: fix example of query-migrate command
  qapi: fix examples of events missing timestamp
  RFC: add a generator for qapi's examples

 qapi/block-core.json         |  46 ++++-----
 qapi/dump.json               |   1 +
 qapi/machine.json            |   8 +-
 qapi/migration.json          |  83 +++++++++++++--
 qapi/net.json                |   2 +-
 qapi/qdev.json               |   3 +-
 qapi/rocker.json             |   3 +-
 qapi/ui.json                 |   7 +-
 scripts/qapi/dumpexamples.py | 194 +++++++++++++++++++++++++++++++++++
 scripts/qapi/main.py         |   2 +
 10 files changed, 304 insertions(+), 45 deletions(-)
 create mode 100644 scripts/qapi/dumpexamples.py

-- 
2.37.2

[PATCH v1 01/16] qapi: fix example of query-ballon command

[Victor Toso]

Example output has an extra ',' delimiter. Fix it.

Problem was noticed when trying to load the example into python's json
library.

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

diff --git a/qapi/machine.json b/qapi/machine.json
index 6afd1936b0..5f1f50d3ed 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1074,7 +1074,7 @@
 #
 # -> { "execute": "query-balloon" }
 # <- { "return": {
-#          "actual": 1073741824,
+#          "actual": 1073741824
 #       }
 #    }
 #
-- 
2.37.2

[PATCH v1 02/16] qapi: fix example of query-vnc command

[Victor Toso]

Example output has an extra ',' delimiter in member "websocket" and it
lacks it in "family" member. Fix it.

Problem was noticed when trying to load the example into python's json
library.

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

diff --git a/qapi/ui.json b/qapi/ui.json
index cf58ab4283..286c5731d1 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -667,8 +667,8 @@
 #             {
 #                "host":"127.0.0.1",
 #                "service":"50401",
-#                "family":"ipv4"
-#                "websocket":false,
+#                "family":"ipv4",
+#                "websocket":false
 #             }
 #          ]
 #       }
-- 
2.37.2

[PATCH v1 03/16] qapi: fix example of query-spice command

[Victor Toso]

Example output has an extra ',' delimiter and a foreign comment
format. Fix it.

Problem was noticed when trying to load the example into python's json
library.

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

diff --git a/qapi/ui.json b/qapi/ui.json
index 286c5731d1..901b84da8a 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -356,8 +356,7 @@
 #                "host": "127.0.0.1",
 #                "channel-id": 0,
 #                "tls": false
-#             },
-#             [ ... more channels follow ... ]
+#             }
 #          ]
 #       }
 #    }
-- 
2.37.2

[PATCH v1 04/16] qapi: fix example of query-rocker-of-dpa-flows command

[Victor Toso]

Example output has an extra ',' delimiter and a foreign comment
format. Fix it.

Problem was noticed when trying to load the example into python's json
library.

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

diff --git a/qapi/rocker.json b/qapi/rocker.json
index b48e49a89b..6ae8689026 100644
--- a/qapi/rocker.json
+++ b/qapi/rocker.json
@@ -251,8 +251,7 @@
 #                   "cookie": 0,
 #                   "action": {"goto-tbl": 10},
 #                   "mask": {"in-pport": 4294901760}
-#                  },
-#                  {...more...},
+#                  }
 #    ]}
 #
 ##
-- 
2.37.2

[PATCH v1 05/16] qapi: fix example of query-dump-guest-memory-capability command

[Victor Toso]

Example output is missing closing curly brackets. Fix it.

Problem was noticed when trying to load the example into python's json
library.

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

diff --git a/qapi/dump.json b/qapi/dump.json
index 90859c5483..22971a9910 100644
--- a/qapi/dump.json
+++ b/qapi/dump.json
@@ -196,6 +196,7 @@
 # -> { "execute": "query-dump-guest-memory-capability" }
 # <- { "return": { "formats":
 #                  ["elf", "kdump-zlib", "kdump-lzo", "kdump-snappy"] }
+#    }
 #
 ##
 { 'command': 'query-dump-guest-memory-capability',
-- 
2.37.2

[PATCH v1 06/16] qapi: fix example of query-blockstats command

[Victor Toso]

Example output is missing several ',' delimiter between members.
Fix it.

Problem was noticed when trying to load the example into python's json
library.

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

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 2173e7734a..72592034b1 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1022,10 +1022,10 @@
 #                   "wr_bytes":9786368,
 #                   "wr_operations":751,
 #                   "rd_bytes":122567168,
-#                   "rd_operations":36772
-#                   "wr_total_times_ns":313253456
-#                   "rd_total_times_ns":3465673657
-#                   "flush_total_times_ns":49653
+#                   "rd_operations":36772,
+#                   "wr_total_times_ns":313253456,
+#                   "rd_total_times_ns":3465673657,
+#                   "flush_total_times_ns":49653,
 #                   "flush_operations":61,
 #                   "rd_merged":0,
 #                   "wr_merged":0,
@@ -1039,10 +1039,10 @@
 #                "wr_bytes":9786368,
 #                "wr_operations":692,
 #                "rd_bytes":122739200,
-#                "rd_operations":36604
+#                "rd_operations":36604,
 #                "flush_operations":51,
-#                "wr_total_times_ns":313253456
-#                "rd_total_times_ns":3465673657
+#                "wr_total_times_ns":313253456,
+#                "rd_total_times_ns":3465673657,
 #                "flush_total_times_ns":49653,
 #                "rd_merged":0,
 #                "wr_merged":0,
@@ -1059,10 +1059,10 @@
 #                "wr_bytes":0,
 #                "wr_operations":0,
 #                "rd_bytes":0,
-#                "rd_operations":0
+#                "rd_operations":0,
 #                "flush_operations":0,
-#                "wr_total_times_ns":0
-#                "rd_total_times_ns":0
+#                "wr_total_times_ns":0,
+#                "rd_total_times_ns":0,
 #                "flush_total_times_ns":0,
 #                "rd_merged":0,
 #                "wr_merged":0,
@@ -1078,10 +1078,10 @@
 #                "wr_bytes":0,
 #                "wr_operations":0,
 #                "rd_bytes":0,
-#                "rd_operations":0
+#                "rd_operations":0,
 #                "flush_operations":0,
-#                "wr_total_times_ns":0
-#                "rd_total_times_ns":0
+#                "wr_total_times_ns":0,
+#                "rd_total_times_ns":0,
 #                "flush_total_times_ns":0,
 #                "rd_merged":0,
 #                "wr_merged":0,
@@ -1097,10 +1097,10 @@
 #                "wr_bytes":0,
 #                "wr_operations":0,
 #                "rd_bytes":0,
-#                "rd_operations":0
+#                "rd_operations":0,
 #                "flush_operations":0,
-#                "wr_total_times_ns":0
-#                "rd_total_times_ns":0
+#                "wr_total_times_ns":0,
+#                "rd_total_times_ns":0,
 #                "flush_total_times_ns":0,
 #                "rd_merged":0,
 #                "wr_merged":0,
-- 
2.37.2

[PATCH v1 07/16] qapi: fix example of BLOCK_JOB_READY event

[Victor Toso]

Example output is missing ',' delimiter. Fix it.

Problem was noticed when trying to load the example into python's json
library.

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 72592034b1..c038d9225d 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -5252,7 +5252,7 @@
 #
 # <- { "event": "BLOCK_JOB_READY",
 #      "data": { "device": "drive0", "type": "mirror", "speed": 0,
-#                "len": 2097152, "offset": 2097152 }
+#                "len": 2097152, "offset": 2097152 },
 #      "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
 #
 ##
-- 
2.37.2

[PATCH v1 08/16] qapi: fix example of NIC_RX_FILTER_CHANGED event

[Victor Toso]

Example output has an extra ending curly bracket. Fix it.

Problem was noticed when trying to load the example into python's json
library.

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

diff --git a/qapi/net.json b/qapi/net.json
index 75ba2cb989..a8ad698e7d 100644
--- a/qapi/net.json
+++ b/qapi/net.json
@@ -756,7 +756,7 @@
 # <- { "event": "NIC_RX_FILTER_CHANGED",
 #      "data": { "name": "vnet0",
 #                "path": "/machine/peripheral/vnet0/virtio-backend" },
-#      "timestamp": { "seconds": 1368697518, "microseconds": 326866 } }
+#      "timestamp": { "seconds": 1368697518, "microseconds": 326866 }
 #    }
 #
 ##
-- 
2.37.2

[PATCH v1 09/16] qapi: fix example of DEVICE_UNPLUG_GUEST_ERROR event

[Victor Toso]

Example output is missing a ',' delimiter and it has an extra ending
curly bracket. Fix it.

Problem was noticed when trying to load the example into python's json
library.

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

diff --git a/qapi/qdev.json b/qapi/qdev.json
index 26cd10106b..2708fb4e99 100644
--- a/qapi/qdev.json
+++ b/qapi/qdev.json
@@ -150,10 +150,9 @@
 #
 # Example:
 #
-# <- { "event": "DEVICE_UNPLUG_GUEST_ERROR"
+# <- { "event": "DEVICE_UNPLUG_GUEST_ERROR",
 #      "data": { "device": "core1",
 #                "path": "/machine/peripheral/core1" },
-#      },
 #      "timestamp": { "seconds": 1615570772, "microseconds": 202844 } }
 #
 ##
-- 
2.37.2

[PATCH v1 10/16] qapi: fix example of MEM_UNPLUG_ERROR event

[Victor Toso]

Example output was missing ',' delimiter. Fix it.

Problem was noticed when trying to load the example into python's json
library.

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

diff --git a/qapi/machine.json b/qapi/machine.json
index 5f1f50d3ed..4782eea2c3 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1416,7 +1416,7 @@
 #
 # Example:
 #
-# <- { "event": "MEM_UNPLUG_ERROR"
+# <- { "event": "MEM_UNPLUG_ERROR",
 #      "data": { "device": "dimm1",
 #                "msg": "acpi: device unplug for unsupported device"
 #      },
-- 
2.37.2

[PATCH v1 11/16] qapi: fix examples of blockdev-add with qcow2

[Victor Toso]

The examples use "qcow2" driver with the wrong member name for
BlockdevRef alternate type. This patch changes all wrong member names
from "file" to "data-file" which is the correct member name in
BlockdevOptionsQcow2 for the BlockdevRef field.

Problem was noticed when using the example as a test case for Go
bindings.

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

diff --git a/qapi/block-core.json b/qapi/block-core.json
index c038d9225d..dcc6d41494 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1541,8 +1541,8 @@
 # -> { "execute": "blockdev-add",
 #      "arguments": { "driver": "qcow2",
 #                     "node-name": "node1534",
-#                     "file": { "driver": "file",
-#                               "filename": "hd1.qcow2" },
+#                     "data-file": { "driver": "file",
+#                                    "filename": "hd1.qcow2" },
 #                     "backing": null } }
 #
 # <- { "return": {} }
@@ -4378,7 +4378,7 @@
 #      "arguments": {
 #           "driver": "qcow2",
 #           "node-name": "test1",
-#           "file": {
+#           "data-file": {
 #               "driver": "file",
 #               "filename": "test.qcow2"
 #            }
@@ -4395,7 +4395,7 @@
 #           "cache": {
 #              "direct": true
 #            },
-#            "file": {
+#           "data-file": {
 #              "driver": "file",
 #              "filename": "/tmp/test.qcow2"
 #            },
@@ -4477,7 +4477,7 @@
 #      "arguments": {
 #           "driver": "qcow2",
 #           "node-name": "node0",
-#           "file": {
+#           "data-file": {
 #               "driver": "file",
 #               "filename": "test.qcow2"
 #           }
-- 
2.37.2

[PATCH v1 12/16] qapi: fix example of blockdev-add command

[Victor Toso]

The example output is setting optional member "backing" with null.
This has no runtime impact. Remove it.

Problem was noticed when using the example as a test case for Go
bindings.

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

diff --git a/qapi/block-core.json b/qapi/block-core.json
index dcc6d41494..302164d575 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1542,9 +1542,7 @@
 #      "arguments": { "driver": "qcow2",
 #                     "node-name": "node1534",
 #                     "data-file": { "driver": "file",
-#                                    "filename": "hd1.qcow2" },
-#                     "backing": null } }
-#
+#                                    "filename": "hd1.qcow2" } } }
 # <- { "return": {} }
 #
 # -> { "execute": "blockdev-snapshot",
-- 
2.37.2

[PATCH v1 13/16] qapi: fix example of query-hotpluggable-cpus command

[Victor Toso]

The example return type has the wrong member name. Fix it.

Problem was noticed when using the example as a test case for Go
bindings.

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

diff --git a/qapi/machine.json b/qapi/machine.json
index 4782eea2c3..abb2f48808 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -958,9 +958,9 @@
 #
 # -> { "execute": "query-hotpluggable-cpus" }
 # <- {"return": [
-#      { "props": { "core": 8 }, "type": "POWER8-spapr-cpu-core",
+#      { "props": { "core-id": 8 }, "type": "POWER8-spapr-cpu-core",
 #        "vcpus-count": 1 },
-#      { "props": { "core": 0 }, "type": "POWER8-spapr-cpu-core",
+#      { "props": { "core-id": 0 }, "type": "POWER8-spapr-cpu-core",
 #        "vcpus-count": 1, "qom-path": "/machine/unattached/device[0]"}
 #    ]}'
 #
-- 
2.37.2

[PATCH v1 14/16] qapi: fix example of query-migrate command

[Victor Toso]

The example's return type has several missing mandatory member names.
Fix it.

Problem was noticed when using the example as a test case for Go
bindings.

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

diff --git a/qapi/migration.json b/qapi/migration.json
index 81185d4311..2ac26d1971 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -295,7 +295,18 @@
 #           "duplicate":123,
 #           "normal":123,
 #           "normal-bytes":123456,
-#           "dirty-sync-count":15
+#           "dirty-sync-count":15,
+#           "mbps":0,
+#           "skipped":0,
+#           "postcopy-requests":0,
+#           "page-size":0,
+#           "multifd-bytes":0,
+#           "pages-per-second":0,
+#           "precopy-bytes":0,
+#           "downtime-bytes":0,
+#           "postcopy-bytes":0,
+#           "dirty-sync-missed-zero-copy":0,
+#           "dirty-pages-rate":0
 #         }
 #      }
 #    }
@@ -321,7 +332,18 @@
 #             "duplicate":123,
 #             "normal":123,
 #             "normal-bytes":123456,
-#             "dirty-sync-count":15
+#             "dirty-sync-count":15,
+#             "mbps":0,
+#             "skipped":0,
+#             "postcopy-requests":0,
+#             "page-size":0,
+#             "multifd-bytes":0,
+#             "pages-per-second":0,
+#             "precopy-bytes":0,
+#             "downtime-bytes":0,
+#             "postcopy-bytes":0,
+#             "dirty-sync-missed-zero-copy":0,
+#             "dirty-pages-rate":0
 #          }
 #       }
 #    }
@@ -342,12 +364,38 @@
 #             "duplicate":123,
 #             "normal":123,
 #             "normal-bytes":123456,
-#             "dirty-sync-count":15
+#             "dirty-sync-count":15,
+#             "mbps":1,
+#             "skipped":1,
+#             "postcopy-requests":1,
+#             "page-size":1,
+#             "multifd-bytes":1,
+#             "pages-per-second":1,
+#             "precopy-bytes":1,
+#             "downtime-bytes":1,
+#             "postcopy-bytes":1,
+#             "dirty-sync-missed-zero-copy":1,
+#             "dirty-pages-rate":1
 #          },
 #          "disk":{
 #             "total":20971520,
 #             "remaining":20880384,
-#             "transferred":91136
+#             "transferred":91136,
+#             "duplicate":2,
+#             "skipped":2,
+#             "normal":2,
+#             "normal-bytes":2,
+#             "dirty-pages-rate":2,
+#             "mbps":2,
+#             "dirty-sync-count":2,
+#             "postcopy-requests":2,
+#             "page-size":2,
+#             "multifd-bytes":2,
+#             "pages-per-second":2,
+#             "precopy-bytes":2,
+#             "downtime-bytes":2,
+#             "postcopy-bytes":2,
+#             "dirty-sync-missed-zero-copy":2
 #          }
 #       }
 #    }
-- 
2.37.2

[PATCH v1 15/16] qapi: fix examples of events missing timestamp

[Victor Toso]

I've used real timestamp and changing them one by one so they would
not be all equal.

Problem was noticed when using the example as a test case for Go
bindings.

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

diff --git a/qapi/migration.json b/qapi/migration.json
index 2ac26d1971..93406b9d51 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -2043,16 +2043,23 @@
 #    }
 # <- { "return": { } }
 # <- {"event": "JOB_STATUS_CHANGE",
+#     "timestamp": {"seconds": 1432121972, "microseconds": 744001},
 #     "data": {"status": "created", "id": "snapsave0"}}
 # <- {"event": "JOB_STATUS_CHANGE",
+#     "timestamp": {"seconds": 1432122172, "microseconds": 744001},
 #     "data": {"status": "running", "id": "snapsave0"}}
-# <- {"event": "STOP"}
-# <- {"event": "RESUME"}
+# <- {"event": "STOP",
+#     "timestamp": {"seconds": 1432122372, "microseconds": 744001} }
+# <- {"event": "RESUME",
+#     "timestamp": {"seconds": 1432122572, "microseconds": 744001} }
 # <- {"event": "JOB_STATUS_CHANGE",
+#     "timestamp": {"seconds": 1432122772, "microseconds": 744001},
 #     "data": {"status": "waiting", "id": "snapsave0"}}
 # <- {"event": "JOB_STATUS_CHANGE",
+#     "timestamp": {"seconds": 1432122972, "microseconds": 744001},
 #     "data": {"status": "pending", "id": "snapsave0"}}
 # <- {"event": "JOB_STATUS_CHANGE",
+#     "timestamp": {"seconds": 1432123172, "microseconds": 744001},
 #     "data": {"status": "concluded", "id": "snapsave0"}}
 # -> {"execute": "query-jobs"}
 # <- {"return": [{"current-progress": 1,
@@ -2104,16 +2111,23 @@
 #    }
 # <- { "return": { } }
 # <- {"event": "JOB_STATUS_CHANGE",
+#     "timestamp": {"seconds": 1472124172, "microseconds": 744001},
 #     "data": {"status": "created", "id": "snapload0"}}
 # <- {"event": "JOB_STATUS_CHANGE",
+#     "timestamp": {"seconds": 1472125172, "microseconds": 744001},
 #     "data": {"status": "running", "id": "snapload0"}}
-# <- {"event": "STOP"}
-# <- {"event": "RESUME"}
+# <- {"event": "STOP",
+#     "timestamp": {"seconds": 1472125472, "microseconds": 744001} }
+# <- {"event": "RESUME",
+#     "timestamp": {"seconds": 1472125872, "microseconds": 744001} }
 # <- {"event": "JOB_STATUS_CHANGE",
+#     "timestamp": {"seconds": 1472126172, "microseconds": 744001},
 #     "data": {"status": "waiting", "id": "snapload0"}}
 # <- {"event": "JOB_STATUS_CHANGE",
+#     "timestamp": {"seconds": 1472127172, "microseconds": 744001},
 #     "data": {"status": "pending", "id": "snapload0"}}
 # <- {"event": "JOB_STATUS_CHANGE",
+#     "timestamp": {"seconds": 1472128172, "microseconds": 744001},
 #     "data": {"status": "concluded", "id": "snapload0"}}
 # -> {"execute": "query-jobs"}
 # <- {"return": [{"current-progress": 1,
@@ -2156,14 +2170,19 @@
 #    }
 # <- { "return": { } }
 # <- {"event": "JOB_STATUS_CHANGE",
+#     "timestamp": {"seconds": 1442124172, "microseconds": 744001},
 #     "data": {"status": "created", "id": "snapdelete0"}}
 # <- {"event": "JOB_STATUS_CHANGE",
+#     "timestamp": {"seconds": 1442125172, "microseconds": 744001},
 #     "data": {"status": "running", "id": "snapdelete0"}}
 # <- {"event": "JOB_STATUS_CHANGE",
+#     "timestamp": {"seconds": 1442126172, "microseconds": 744001},
 #     "data": {"status": "waiting", "id": "snapdelete0"}}
 # <- {"event": "JOB_STATUS_CHANGE",
+#     "timestamp": {"seconds": 1442127172, "microseconds": 744001},
 #     "data": {"status": "pending", "id": "snapdelete0"}}
 # <- {"event": "JOB_STATUS_CHANGE",
+#     "timestamp": {"seconds": 1442128172, "microseconds": 744001},
 #     "data": {"status": "concluded", "id": "snapdelete0"}}
 # -> {"execute": "query-jobs"}
 # <- {"return": [{"current-progress": 1,
-- 
2.37.2

[PATCH v1 16/16] RFC: add a generator for qapi's examples

[Victor Toso]

The goal of this generator is to validate QAPI examples and transform
them into a format that can be used for 3rd party applications to
validate their QAPI/QMP introspection.

For each Example section, we parse server and client messages into a
python dictionary. This step alone has found several ill formatted
JSON messages in the examples.

The generator outputs another JSON file with all the examples in the
QAPI module that they came from. This can be used to validate the
introspection between QAPI/QMP to language bindings.

When used with the POC qapi-go branch, we have found bad QMP messages
with wrong member names, mandatory members that were missing and
optional members that were being set with null (not needed).

A simple example of the output format is:

 { "examples": [
   {
     "id": "ksuxwzfayw",
     "client": [
     {
       "sequence-order": 1
       "message-type": "command",
       "message":
       { "arguments":
         { "device": "scratch", "size": 1073741824 },
         "execute": "block_resize"
       },
    } ],
    "server": [
    {
      "sequence-order": 2
      "message-type": "return",
      "message": { "return": {} },
    } ]
    }
  ] }

If this idea seems reasonable, we can add python-qemu-qmp to validate
each message at generation time already.

Signed-off-by: Victor Toso <victortoso@redhat.com>
---
 scripts/qapi/dumpexamples.py | 194 +++++++++++++++++++++++++++++++++++
 scripts/qapi/main.py         |   2 +
 2 files changed, 196 insertions(+)
 create mode 100644 scripts/qapi/dumpexamples.py

diff --git a/scripts/qapi/dumpexamples.py b/scripts/qapi/dumpexamples.py
new file mode 100644
index 0000000000..c14ed11774
--- /dev/null
+++ b/scripts/qapi/dumpexamples.py
@@ -0,0 +1,194 @@
+"""
+Dump examples for Developers
+"""
+# Copyright (c) 2022 Red Hat Inc.
+#
+# Authors:
+#  Victor Toso <victortoso@redhat.com>
+#
+# This work is licensed under the terms of the GNU GPL, version 2.
+# See the COPYING file in the top-level directory.
+
+# Just for type hint on self
+from __future__ import annotations
+
+import os
+import json
+import random
+import string
+
+from typing import Dict, List, Optional
+
+from .schema import (
+    QAPISchema,
+    QAPISchemaType,
+    QAPISchemaVisitor,
+    QAPISchemaEnumMember,
+    QAPISchemaFeature,
+    QAPISchemaIfCond,
+    QAPISchemaObjectType,
+    QAPISchemaObjectTypeMember,
+    QAPISchemaVariants,
+)
+from .source import QAPISourceInfo
+
+
+def gen_examples(schema: QAPISchema,
+                 output_dir: str,
+                 prefix: str) -> None:
+    vis = QAPISchemaGenExamplesVisitor(prefix)
+    schema.visit(vis)
+    vis.write(output_dir)
+
+
+def get_id(random, size: int) -> str:
+    letters = string.ascii_lowercase
+    return ''.join(random.choice(letters) for i in range(size))
+
+
+def next_object(text, start, end, context) -> Dict:
+    # Start of json object
+    start = text.find("{", start)
+    end = text.rfind("}", start, end+1)
+
+    # try catch, pretty print issues
+    try:
+        ret = json.loads(text[start:end+1])
+    except Exception as e:
+        print("Error: {}\nLocation: {}\nData: {}\n".format(
+              str(e), context, text[start:end+1]))
+        return {}
+    else:
+        return ret
+
+
+def parse_text_to_dicts(text: str, context: str) -> List[Dict]:
+    examples, clients, servers = [], [], []
+
+    count = 1
+    c, s = text.find("->"), text.find("<-")
+    while c != -1 or s != -1:
+        if c == -1 or (s != -1 and s < c):
+            start, target = s, servers
+        else:
+            start, target = c, clients
+
+        # Find the client and server, if any
+        if c != -1:
+            c = text.find("->", start + 1)
+        if s != -1:
+            s = text.find("<-", start + 1)
+
+        # Find the limit of current's object.
+        # We first look for the next message, either client or server. If none
+        # is avaible, we set the end of the text as limit.
+        if c == -1 and s != -1:
+            end = s
+        elif c != -1 and s == -1:
+            end = c
+        elif c != -1 and s != -1:
+            end = (c < s) and c or s
+        else:
+            end = len(text) - 1
+
+        message = next_object(text, start, end, context)
+        if len(message) > 0:
+            message_type = "return"
+            if "execute" in message:
+                message_type = "command"
+            elif "event" in message:
+                message_type = "event"
+
+            target.append({
+                "sequence-order": count,
+                "message-type": message_type,
+                "message": message
+            })
+            count += 1
+
+    examples.append({"client": clients, "server": servers})
+    return examples
+
+
+def parse_examples_of(self: QAPISchemaGenExamplesVisitor,
+                      name: str):
+
+    assert(name in self.schema._entity_dict)
+    obj = self.schema._entity_dict[name]
+    assert((obj.doc is not None))
+    module_name = obj._module.name
+
+    # We initialize random with the name so that we get consistent example
+    # ids over different generations. The ids of a given example might
+    # change when adding/removing examples, but that's acceptable as the
+    # goal is just to grep $id to find what example failed at a given test
+    # with minimum chorn over regenerating.
+    random.seed(name, version=2)
+
+    for s in obj.doc.sections:
+        if s.name != "Example":
+            continue
+
+        if module_name not in self.target:
+            self.target[module_name] = []
+
+        context = f'''{name} at {obj.info.fname}:{obj.info.line}'''
+        examples = parse_text_to_dicts(s.text, context)
+        for example in examples:
+            self.target[module_name].append({
+                    "id": get_id(random, 10),
+                    "client": example["client"],
+                    "server": example["server"]
+            })
+
+
+class QAPISchemaGenExamplesVisitor(QAPISchemaVisitor):
+
+    def __init__(self, prefix: str):
+        super().__init__()
+        self.target = {}
+        self.schema = None
+
+    def visit_begin(self, schema):
+        self.schema = schema
+
+    def visit_end(self):
+        self.schema = None
+
+    def write(self: QAPISchemaGenExamplesVisitor,
+              output_dir: str) -> None:
+        for filename, content in self.target.items():
+            pathname = os.path.join(output_dir, "examples", filename)
+            odir = os.path.dirname(pathname)
+            os.makedirs(odir, exist_ok=True)
+            result = {"examples": content}
+
+            with open(pathname, "w") as outfile:
+                outfile.write(json.dumps(result, indent=2, sort_keys=True))
+
+    def visit_command(self: QAPISchemaGenExamplesVisitor,
+                      name: str,
+                      info: Optional[QAPISourceInfo],
+                      ifcond: QAPISchemaIfCond,
+                      features: List[QAPISchemaFeature],
+                      arg_type: Optional[QAPISchemaObjectType],
+                      ret_type: Optional[QAPISchemaType],
+                      gen: bool,
+                      success_response: bool,
+                      boxed: bool,
+                      allow_oob: bool,
+                      allow_preconfig: bool,
+                      coroutine: bool) -> None:
+
+        if gen:
+            parse_examples_of(self, name)
+
+    def visit_event(self: QAPISchemaGenExamplesVisitor,
+                    name: str,
+                    info: Optional[QAPISourceInfo],
+                    ifcond: QAPISchemaIfCond,
+                    features: List[QAPISchemaFeature],
+                    arg_type: Optional[QAPISchemaObjectType],
+                    boxed: bool):
+
+        parse_examples_of(self, name)
diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py
index fc216a53d3..9e771f4dd3 100644
--- a/scripts/qapi/main.py
+++ b/scripts/qapi/main.py
@@ -13,6 +13,7 @@
 
 from .commands import gen_commands
 from .common import must_match
+from .dumpexamples import gen_examples
 from .error import QAPIError
 from .events import gen_events
 from .introspect import gen_introspect
@@ -54,6 +55,7 @@ def generate(schema_file: str,
     gen_events(schema, output_dir, prefix)
     gen_introspect(schema, output_dir, prefix, unmask)
 
+    gen_examples(schema, output_dir, prefix)
 
 def main() -> int:
     """
-- 
2.37.2

Re: [PATCH v1 08/16] qapi: fix example of NIC_RX_FILTER_CHANGED event

[Markus Armbruster]

Victor Toso <victortoso@redhat.com> writes:

> Example output has an extra ending curly bracket. Fix it.
>
> Problem was noticed when trying to load the example into python's json
> library.
>
> Signed-off-by: Victor Toso <victortoso@redhat.com>
> ---
>  qapi/net.json | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/qapi/net.json b/qapi/net.json
> index 75ba2cb989..a8ad698e7d 100644
> --- a/qapi/net.json
> +++ b/qapi/net.json
> @@ -756,7 +756,7 @@
>  # <- { "event": "NIC_RX_FILTER_CHANGED",
>  #      "data": { "name": "vnet0",
>  #                "path": "/machine/peripheral/vnet0/virtio-backend" },
> -#      "timestamp": { "seconds": 1368697518, "microseconds": 326866 } }
> +#      "timestamp": { "seconds": 1368697518, "microseconds": 326866 }
>  #    }
>  #
>  ##

Again, for local consistency:

diff --git a/qapi/net.json b/qapi/net.json
index 75ba2cb989..dd088c09c5 100644
--- a/qapi/net.json
+++ b/qapi/net.json
@@ -757,7 +757,6 @@
 #      "data": { "name": "vnet0",
 #                "path": "/machine/peripheral/vnet0/virtio-backend" },
 #      "timestamp": { "seconds": 1368697518, "microseconds": 326866 } }
-#    }
 #
 ##
 { 'event': 'NIC_RX_FILTER_CHANGED',

Re: [PATCH v1 12/16] qapi: fix example of blockdev-add command

[Markus Armbruster]

Victor Toso <victortoso@redhat.com> writes:

> The example output is setting optional member "backing" with null.
> This has no runtime impact. Remove it.
>
> Problem was noticed when using the example as a test case for Go
> bindings.

"Fix example" and "problem" implies there's something wrong.  "No
runtime impact" sounds like it's redundant, but not wrong.  Wrong or not
wrong?

> Signed-off-by: Victor Toso <victortoso@redhat.com>
> ---
>  qapi/block-core.json | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index dcc6d41494..302164d575 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1542,9 +1542,7 @@
>  #      "arguments": { "driver": "qcow2",
>  #                     "node-name": "node1534",
>  #                     "data-file": { "driver": "file",
> -#                                    "filename": "hd1.qcow2" },
> -#                     "backing": null } }
> -#
> +#                                    "filename": "hd1.qcow2" } } }
>  # <- { "return": {} }
>  #
>  # -> { "execute": "blockdev-snapshot",

Re: [PATCH v1 03/16] qapi: fix example of query-spice command

[Markus Armbruster]

Victor Toso <victortoso@redhat.com> writes:

> Example output has an extra ',' delimiter and a foreign comment
> format. Fix it.
>
> Problem was noticed when trying to load the example into python's json
> library.
>
> Signed-off-by: Victor Toso <victortoso@redhat.com>
> ---
>  qapi/ui.json | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/qapi/ui.json b/qapi/ui.json
> index 286c5731d1..901b84da8a 100644
> --- a/qapi/ui.json
> +++ b/qapi/ui.json
> @@ -356,8 +356,7 @@
>  #                "host": "127.0.0.1",
>  #                "channel-id": 0,
>  #                "tls": false
> -#             },
> -#             [ ... more channels follow ... ]
> +#             }
>  #          ]
>  #       }
>  #    }

Hmm.  You're removing an ellipsis Gerd put there for a reason, I presume
(commit cb42a870c3 "spice: add qmp 'query-spice' and hmp 'info spice'
commands.")

Even if we can do without it here, We may still want a way to abridge
examples.  Thoughts?

Re: [PATCH v1 04/16] qapi: fix example of query-rocker-of-dpa-flows command

[Markus Armbruster]

Victor Toso <victortoso@redhat.com> writes:

> Example output has an extra ',' delimiter and a foreign comment
> format. Fix it.
>
> Problem was noticed when trying to load the example into python's json
> library.
>
> Signed-off-by: Victor Toso <victortoso@redhat.com>
> ---
>  qapi/rocker.json | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/qapi/rocker.json b/qapi/rocker.json
> index b48e49a89b..6ae8689026 100644
> --- a/qapi/rocker.json
> +++ b/qapi/rocker.json
> @@ -251,8 +251,7 @@
>  #                   "cookie": 0,
>  #                   "action": {"goto-tbl": 10},
>  #                   "mask": {"in-pport": 4294901760}
> -#                  },
> -#                  {...more...},
> +#                  }
>  #    ]}
>  #
>  ##

Review of previous patch applies.

Re: [PATCH v1 14/16] qapi: fix example of query-migrate command

[Markus Armbruster]

Victor Toso <victortoso@redhat.com> writes:

> The example's return type has several missing mandatory member names.
> Fix it.
>
> Problem was noticed when using the example as a test case for Go
> bindings.
>
> Signed-off-by: Victor Toso <victortoso@redhat.com>
> ---
>  qapi/migration.json | 56 +++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 52 insertions(+), 4 deletions(-)
>
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 81185d4311..2ac26d1971 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -295,7 +295,18 @@
>  #           "duplicate":123,
>  #           "normal":123,
>  #           "normal-bytes":123456,
> -#           "dirty-sync-count":15
> +#           "dirty-sync-count":15,
> +#           "mbps":0,
> +#           "skipped":0,
> +#           "postcopy-requests":0,
> +#           "page-size":0,
> +#           "multifd-bytes":0,
> +#           "pages-per-second":0,
> +#           "precopy-bytes":0,
> +#           "downtime-bytes":0,
> +#           "postcopy-bytes":0,
> +#           "dirty-sync-missed-zero-copy":0,
> +#           "dirty-pages-rate":0
>  #         }
>  #      }
>  #    }
> @@ -321,7 +332,18 @@
>  #             "duplicate":123,
>  #             "normal":123,
>  #             "normal-bytes":123456,
> -#             "dirty-sync-count":15
> +#             "dirty-sync-count":15,
> +#             "mbps":0,
> +#             "skipped":0,
> +#             "postcopy-requests":0,
> +#             "page-size":0,
> +#             "multifd-bytes":0,
> +#             "pages-per-second":0,
> +#             "precopy-bytes":0,
> +#             "downtime-bytes":0,
> +#             "postcopy-bytes":0,
> +#             "dirty-sync-missed-zero-copy":0,
> +#             "dirty-pages-rate":0
>  #          }
>  #       }
>  #    }
> @@ -342,12 +364,38 @@
>  #             "duplicate":123,
>  #             "normal":123,
>  #             "normal-bytes":123456,
> -#             "dirty-sync-count":15
> +#             "dirty-sync-count":15,
> +#             "mbps":1,
> +#             "skipped":1,
> +#             "postcopy-requests":1,
> +#             "page-size":1,
> +#             "multifd-bytes":1,
> +#             "pages-per-second":1,
> +#             "precopy-bytes":1,
> +#             "downtime-bytes":1,
> +#             "postcopy-bytes":1,
> +#             "dirty-sync-missed-zero-copy":1,
> +#             "dirty-pages-rate":1
>  #          },
>  #          "disk":{
>  #             "total":20971520,
>  #             "remaining":20880384,
> -#             "transferred":91136
> +#             "transferred":91136,
> +#             "duplicate":2,
> +#             "skipped":2,
> +#             "normal":2,
> +#             "normal-bytes":2,
> +#             "dirty-pages-rate":2,
> +#             "mbps":2,
> +#             "dirty-sync-count":2,
> +#             "postcopy-requests":2,
> +#             "page-size":2,
> +#             "multifd-bytes":2,
> +#             "pages-per-second":2,
> +#             "precopy-bytes":2,
> +#             "downtime-bytes":2,
> +#             "postcopy-bytes":2,
> +#             "dirty-sync-missed-zero-copy":2
>  #          }
>  #       }
>  #    }

Full example output is fairly long.  Perhaps that's what we want.  Or
perhaps we'd prefer abridged output.  See also my review of PATCH 03.

Re: [PATCH v1 16/16] RFC: add a generator for qapi's examples

[Markus Armbruster]

Victor Toso <victortoso@redhat.com> writes:

> The goal of this generator is to validate QAPI examples and transform
> them into a format that can be used for 3rd party applications to
> validate their QAPI/QMP introspection.
>
> For each Example section, we parse server and client messages into a
> python dictionary. This step alone has found several ill formatted
> JSON messages in the examples.
>
> The generator outputs another JSON file with all the examples in the
> QAPI module that they came from. This can be used to validate the
> introspection between QAPI/QMP to language bindings.
>
> When used with the POC qapi-go branch, we have found bad QMP messages
> with wrong member names, mandatory members that were missing and
> optional members that were being set with null (not needed).
>
> A simple example of the output format is:
>
>  { "examples": [
>    {
>      "id": "ksuxwzfayw",
>      "client": [
>      {
>        "sequence-order": 1
>        "message-type": "command",
>        "message":
>        { "arguments":
>          { "device": "scratch", "size": 1073741824 },
>          "execute": "block_resize"
>        },
>     } ],
>     "server": [
>     {
>       "sequence-order": 2
>       "message-type": "return",
>       "message": { "return": {} },
>     } ]
>     }
>   ] }
>
> If this idea seems reasonable, we can add python-qemu-qmp to validate
> each message at generation time already.
>
> Signed-off-by: Victor Toso <victortoso@redhat.com>

If I understand you correctly, there are two benefits:

1. Mechanical syntax check for examples

   Love it.

2. Can extract examples for use as test cases

   Sounds good to me.  Possible redundancy with existing tests.
   Probably nothing to worry about.

   Can you explain in a bit more detail how the extracted data is (to
   be) used?

Re: [PATCH v1 12/16] qapi: fix example of blockdev-add command

[Victor Toso]

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

Hi,

On Wed, Aug 31, 2022 at 01:40:50PM +0200, Markus Armbruster wrote:
> Victor Toso <victortoso@redhat.com> writes:
>
> > The example output is setting optional member "backing" with null.
> > This has no runtime impact. Remove it.
> >
> > Problem was noticed when using the example as a test case for Go
> > bindings.
>
> "Fix example" and "problem" implies there's something wrong.
> "No runtime impact" sounds like it's redundant, but not wrong.
> Wrong or not wrong?

I take your comment is more about the wording which is confusing.

Would it be better if I change to:
'''
   The example output is setting optional member "backing" with
   null. While this has no runtime impact, setting optional
   members with empty value should not be encouraged. Remove it.
'''

While I think the above is true, my main reason for proposing
this change is to re-use the example as a test case, but I'm not
sure if adding anything related to it would make it better (only
more confusing!).

Cheers,
Victor

> > Signed-off-by: Victor Toso <victortoso@redhat.com>
> > ---
> >  qapi/block-core.json | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index dcc6d41494..302164d575 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -1542,9 +1542,7 @@
> >  #      "arguments": { "driver": "qcow2",
> >  #                     "node-name": "node1534",
> >  #                     "data-file": { "driver": "file",
> > -#                                    "filename": "hd1.qcow2" },
> > -#                     "backing": null } }
> > -#
> > +#                                    "filename": "hd1.qcow2" } } }
> >  # <- { "return": {} }
> >  #
> >  # -> { "execute": "blockdev-snapshot",
> 

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

Re: [PATCH v1 03/16] qapi: fix example of query-spice command

[Victor Toso]

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

Hi,

On Wed, Aug 31, 2022 at 01:50:31PM +0200, Markus Armbruster wrote:
> Victor Toso <victortoso@redhat.com> writes:
>
> > Example output has an extra ',' delimiter and a foreign comment
> > format. Fix it.
> >
> > Problem was noticed when trying to load the example into python's json
> > library.
> >
> > Signed-off-by: Victor Toso <victortoso@redhat.com>
> > ---
> >  qapi/ui.json | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/qapi/ui.json b/qapi/ui.json
> > index 286c5731d1..901b84da8a 100644
> > --- a/qapi/ui.json
> > +++ b/qapi/ui.json
> > @@ -356,8 +356,7 @@
> >  #                "host": "127.0.0.1",
> >  #                "channel-id": 0,
> >  #                "tls": false
> > -#             },
> > -#             [ ... more channels follow ... ]
> > +#             }
> >  #          ]
> >  #       }
> >  #    }
>
> Hmm.  You're removing an ellipsis Gerd put there for a reason,
> I presume (commit cb42a870c3 "spice: add qmp 'query-spice' and
> hmp 'info spice' commands.")

I guess the reason is just that we get a too big array of
SpiceChannels so the example output would be big but not
interesting.

> Even if we can do without it here, We may still want a way to
> abridge examples.  Thoughts?

I just want something that can be a valid QMP message so we can
transform JSON to a specific language type and then back to QMP
message and compare that both matches.

I don't think that, for the purpose of query-spice documentation
it is worth to have a very similar but with 10 or more entries of
array of channels. So, I'd say it is fine to simply cut it here.
If we have a reason to put it out a 100% valid message, well, we
would have that reason to back it up... but I don't have one.

Cheers,
Victor

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

Re: [PATCH v1 12/16] qapi: fix example of blockdev-add command

[Markus Armbruster]

Cc: Kevin for an improved chance of getting any nonsense I might write
corrected.

Victor Toso <victortoso@redhat.com> writes:

> Hi,
>
> On Wed, Aug 31, 2022 at 01:40:50PM +0200, Markus Armbruster wrote:
>> Victor Toso <victortoso@redhat.com> writes:
>>
>> > The example output is setting optional member "backing" with null.
>> > This has no runtime impact. Remove it.
>> >
>> > Problem was noticed when using the example as a test case for Go
>> > bindings.
>>
>> "Fix example" and "problem" implies there's something wrong.
>> "No runtime impact" sounds like it's redundant, but not wrong.
>> Wrong or not wrong?
>
> I take your comment is more about the wording which is confusing.
>
> Would it be better if I change to:
> '''
>    The example output is setting optional member "backing" with
>    null. While this has no runtime impact, setting optional
>    members with empty value should not be encouraged. Remove it.
> '''
>
> While I think the above is true, my main reason for proposing
> this change is to re-use the example as a test case, but I'm not
> sure if adding anything related to it would make it better (only
> more confusing!).

I had a closer look at the schema.

The definition of backing is

    ##
    # @BlockdevOptionsGenericCOWFormat:
    #
    # Driver specific block device options for image format that have no option
    # besides their data source and an optional backing file.
    #
    # @backing: reference to or definition of the backing file block
    #           device, null disables the backing file entirely.
    #           Defaults to the backing file stored the image file.
    #
    # Since: 2.9
    ##
    { 'struct': 'BlockdevOptionsGenericCOWFormat',
      'base': 'BlockdevOptionsGenericFormat',
      'data': { '*backing': 'BlockdevRefOrNull' } }

Meaning, if I remember correctly (with some help from commit
c42e8742f52's message):

1. Present @backing

1.a. of type 'str' means use the existing block device with this ID as
     backing image

1.b. of type 'BlockdevOptions' means use the new block device defined by
     it as backing image

1.c. that is null means use no backing image

2. Absent @backing means default to the backing file named in the COW
   image.

Therefore, ...

>
> Cheers,
> Victor
>
>> > Signed-off-by: Victor Toso <victortoso@redhat.com>
>> > ---
>> >  qapi/block-core.json | 4 +---
>> >  1 file changed, 1 insertion(+), 3 deletions(-)
>> >
>> > diff --git a/qapi/block-core.json b/qapi/block-core.json
>> > index dcc6d41494..302164d575 100644
>> > --- a/qapi/block-core.json
>> > +++ b/qapi/block-core.json
>> > @@ -1542,9 +1542,7 @@
>> >  #      "arguments": { "driver": "qcow2",
>> >  #                     "node-name": "node1534",
>> >  #                     "data-file": { "driver": "file",
>> > -#                                    "filename": "hd1.qcow2" },
>> > -#                     "backing": null } }
>> > -#
>> > +#                                    "filename": "hd1.qcow2" } } }
>> >  # <- { "return": {} }
>> >  #
>> >  # -> { "execute": "blockdev-snapshot",
>> 

... your patch changes the example from 1.c. to 2.  Which is probably
not what you intended.

Re: [PATCH v1 03/16] qapi: fix example of query-spice command

[Markus Armbruster]

Victor Toso <victortoso@redhat.com> writes:

> Hi,
>
> On Wed, Aug 31, 2022 at 01:50:31PM +0200, Markus Armbruster wrote:
>> Victor Toso <victortoso@redhat.com> writes:
>>
>> > Example output has an extra ',' delimiter and a foreign comment
>> > format. Fix it.
>> >
>> > Problem was noticed when trying to load the example into python's json
>> > library.
>> >
>> > Signed-off-by: Victor Toso <victortoso@redhat.com>
>> > ---
>> >  qapi/ui.json | 3 +--
>> >  1 file changed, 1 insertion(+), 2 deletions(-)
>> >
>> > diff --git a/qapi/ui.json b/qapi/ui.json
>> > index 286c5731d1..901b84da8a 100644
>> > --- a/qapi/ui.json
>> > +++ b/qapi/ui.json
>> > @@ -356,8 +356,7 @@
>> >  #                "host": "127.0.0.1",
>> >  #                "channel-id": 0,
>> >  #                "tls": false
>> > -#             },
>> > -#             [ ... more channels follow ... ]
>> > +#             }
>> >  #          ]
>> >  #       }
>> >  #    }
>>
>> Hmm.  You're removing an ellipsis Gerd put there for a reason,
>> I presume (commit cb42a870c3 "spice: add qmp 'query-spice' and
>> hmp 'info spice' commands.")
>
> I guess the reason is just that we get a too big array of
> SpiceChannels so the example output would be big but not
> interesting.
>
>> Even if we can do without it here, We may still want a way to
>> abridge examples.  Thoughts?
>
> I just want something that can be a valid QMP message so we can
> transform JSON to a specific language type and then back to QMP
> message and compare that both matches.
>
> I don't think that, for the purpose of query-spice documentation
> it is worth to have a very similar but with 10 or more entries of
> array of channels. So, I'd say it is fine to simply cut it here.
> If we have a reason to put it out a 100% valid message, well, we
> would have that reason to back it up... but I don't have one.

I agree listing more channels in the example wouldn't help users.

But do we want to indicate that the example is abridged?

Gerd, I'd like to have your Acked-by for this patch.

Re: [PATCH v1 16/16] RFC: add a generator for qapi's examples

[Victor Toso]

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

Hi,

On Wed, Aug 31, 2022 at 02:01:54PM +0200, Markus Armbruster wrote:
> Victor Toso <victortoso@redhat.com> writes:
>
> > The goal of this generator is to validate QAPI examples and transform
> > them into a format that can be used for 3rd party applications to
> > validate their QAPI/QMP introspection.
> >
> > For each Example section, we parse server and client messages into a
> > python dictionary. This step alone has found several ill formatted
> > JSON messages in the examples.
> >
> > The generator outputs another JSON file with all the examples in the
> > QAPI module that they came from. This can be used to validate the
> > introspection between QAPI/QMP to language bindings.
> >
> > When used with the POC qapi-go branch, we have found bad QMP messages
> > with wrong member names, mandatory members that were missing and
> > optional members that were being set with null (not needed).
> >
> > A simple example of the output format is:
> >
> >  { "examples": [
> >    {
> >      "id": "ksuxwzfayw",
> >      "client": [
> >      {
> >        "sequence-order": 1
> >        "message-type": "command",
> >        "message":
> >        { "arguments":
> >          { "device": "scratch", "size": 1073741824 },
> >          "execute": "block_resize"
> >        },
> >     } ],
> >     "server": [
> >     {
> >       "sequence-order": 2
> >       "message-type": "return",
> >       "message": { "return": {} },
> >     } ]
> >     }
> >   ] }
> >
> > If this idea seems reasonable, we can add python-qemu-qmp to validate
> > each message at generation time already.
> >
> > Signed-off-by: Victor Toso <victortoso@redhat.com>
>
> If I understand you correctly, there are two benefits:
>
> 1. Mechanical syntax check for examples
>
>    Love it.

Not just JSON syntax but can be extend to the introspection
layer. Errors like wrong member names would fail while parsing
the examples (issues such as fixed by patches 11 and 13/16 should
not happen anymore).

> 2. Can extract examples for use as test cases
>
>    Sounds good to me.  Possible redundancy with existing tests.
>    Probably nothing to worry about.
>
>    Can you explain in a bit more detail how the extracted data
>    is (to be) used?

Sure.

The Golang test that consumes this is 152 lines of code [0]. The
idea is that we can use the examples to feed Golang unmarshalling
code and then marshall it back to JSON and compare input JSON
with output JSON and see that their content matches.

[0] https://gitlab.com/victortoso/qapi-go/-/blob/wip-v3/test/examples_test.go

I have generated the examples with this patch series and stored
the output here [1]

[1] https://gitlab.com/victortoso/qapi-go/-/tree/wip-v3/test/data/examples

The examples are QMP messages that are either sent by Client "->"
or sent by Server "<-". The order matters so I take the order set
in the examples and store it as "sequence-order".

In the Go test code, I follow the sequence-order. One example of
this being useful is that we know which Return type to expect
after a Command is issued.

I've also included metadata about the type of message, which is
one of three options: command, event or return. (Errors are
return too).

This is important because it makes the tests very easy to write.
Different Unmarshal/Marshal code can be set in the code block of
the specific message type.

--

The things that makes me quite excited with this idea are:

 1. We have valid functional examples documented. If the examples
    break, we would have the software in place to know it (plug
    to ci or some other ninja check seems reasonable to me)

 2. Developers should get more interested in documenting examples
    as that alone is is a valid test case, even if only useful
    for language binding's syntax.

Cheers,
Victor

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

Re: [PATCH v1 12/16] qapi: fix example of blockdev-add command

[Victor Toso]

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

Hi,

On Wed, Aug 31, 2022 at 03:16:54PM +0200, Markus Armbruster wrote:
> Cc: Kevin for an improved chance of getting any nonsense I might write
> corrected.
>
> Victor Toso <victortoso@redhat.com> writes:
>
> > Hi,
> >
> > On Wed, Aug 31, 2022 at 01:40:50PM +0200, Markus Armbruster wrote:
> >> Victor Toso <victortoso@redhat.com> writes:
> >>
> >> > The example output is setting optional member "backing" with null.
> >> > This has no runtime impact. Remove it.
> >> >
> >> > Problem was noticed when using the example as a test case for Go
> >> > bindings.
> >>
> >> "Fix example" and "problem" implies there's something wrong.
> >> "No runtime impact" sounds like it's redundant, but not wrong.
> >> Wrong or not wrong?
> >
> > I take your comment is more about the wording which is confusing.
> >
> > Would it be better if I change to:
> > '''
> >    The example output is setting optional member "backing" with
> >    null. While this has no runtime impact, setting optional
> >    members with empty value should not be encouraged. Remove it.
> > '''
> >
> > While I think the above is true, my main reason for proposing
> > this change is to re-use the example as a test case, but I'm not
> > sure if adding anything related to it would make it better (only
> > more confusing!).
> 
> I had a closer look at the schema.
> 
> The definition of backing is
> 
>     ##
>     # @BlockdevOptionsGenericCOWFormat:
>     #
>     # Driver specific block device options for image format that have no option
>     # besides their data source and an optional backing file.
>     #
>     # @backing: reference to or definition of the backing file block
>     #           device, null disables the backing file entirely.
>     #           Defaults to the backing file stored the image file.
>     #
>     # Since: 2.9
>     ##
>     { 'struct': 'BlockdevOptionsGenericCOWFormat',
>       'base': 'BlockdevOptionsGenericFormat',
>       'data': { '*backing': 'BlockdevRefOrNull' } }
> 
> Meaning, if I remember correctly (with some help from commit
> c42e8742f52's message):
> 
> 1. Present @backing
> 
> 1.a. of type 'str' means use the existing block device with this ID as
>      backing image
> 
> 1.b. of type 'BlockdevOptions' means use the new block device defined by
>      it as backing image
> 
> 1.c. that is null means use no backing image
>
> 2. Absent @backing means default to the backing file named in the COW
>    image.

Over the wire, how you get the difference between 1.c and 2? Are
you saying that for optional member "backing" we should be
explicit sending null over the wire?

> Therefore, ...
>
> >
> > Cheers,
> > Victor
> >
> >> > Signed-off-by: Victor Toso <victortoso@redhat.com>
> >> > ---
> >> >  qapi/block-core.json | 4 +---
> >> >  1 file changed, 1 insertion(+), 3 deletions(-)
> >> >
> >> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> >> > index dcc6d41494..302164d575 100644
> >> > --- a/qapi/block-core.json
> >> > +++ b/qapi/block-core.json
> >> > @@ -1542,9 +1542,7 @@
> >> >  #      "arguments": { "driver": "qcow2",
> >> >  #                     "node-name": "node1534",
> >> >  #                     "data-file": { "driver": "file",
> >> > -#                                    "filename": "hd1.qcow2" },
> >> > -#                     "backing": null } }
> >> > -#
> >> > +#                                    "filename": "hd1.qcow2" } } }
> >> >  # <- { "return": {} }
> >> >  #
> >> >  # -> { "execute": "blockdev-snapshot",
> >>
>
> ... your patch changes the example from 1.c. to 2.  Which is probably
> not what you intended.

Yep, you are correct but not just with my patch. It is confusing
that an optional member must be set to JSON Null over the wire..
I'll need to think a bit more on this.

Many many thanks for your review. Really appreciate it.

Cheers,
Victor

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

Re: [PATCH v1 12/16] qapi: fix example of blockdev-add command

[Markus Armbruster]

Victor Toso <victortoso@redhat.com> writes:

> Hi,
>
> On Wed, Aug 31, 2022 at 03:16:54PM +0200, Markus Armbruster wrote:
>> Cc: Kevin for an improved chance of getting any nonsense I might write
>> corrected.
>>
>> Victor Toso <victortoso@redhat.com> writes:
>>
>> > Hi,
>> >
>> > On Wed, Aug 31, 2022 at 01:40:50PM +0200, Markus Armbruster wrote:
>> >> Victor Toso <victortoso@redhat.com> writes:
>> >>
>> >> > The example output is setting optional member "backing" with null.
>> >> > This has no runtime impact. Remove it.
>> >> >
>> >> > Problem was noticed when using the example as a test case for Go
>> >> > bindings.
>> >>
>> >> "Fix example" and "problem" implies there's something wrong.
>> >> "No runtime impact" sounds like it's redundant, but not wrong.
>> >> Wrong or not wrong?
>> >
>> > I take your comment is more about the wording which is confusing.
>> >
>> > Would it be better if I change to:
>> > '''
>> >    The example output is setting optional member "backing" with
>> >    null. While this has no runtime impact, setting optional
>> >    members with empty value should not be encouraged. Remove it.
>> > '''
>> >
>> > While I think the above is true, my main reason for proposing
>> > this change is to re-use the example as a test case, but I'm not
>> > sure if adding anything related to it would make it better (only
>> > more confusing!).
>> 
>> I had a closer look at the schema.
>> 
>> The definition of backing is
>> 
>>     ##
>>     # @BlockdevOptionsGenericCOWFormat:
>>     #
>>     # Driver specific block device options for image format that have no option
>>     # besides their data source and an optional backing file.
>>     #
>>     # @backing: reference to or definition of the backing file block
>>     #           device, null disables the backing file entirely.
>>     #           Defaults to the backing file stored the image file.
>>     #
>>     # Since: 2.9
>>     ##
>>     { 'struct': 'BlockdevOptionsGenericCOWFormat',
>>       'base': 'BlockdevOptionsGenericFormat',
>>       'data': { '*backing': 'BlockdevRefOrNull' } }
>> 
>> Meaning, if I remember correctly (with some help from commit
>> c42e8742f52's message):
>> 
>> 1. Present @backing
>> 
>> 1.a. of type 'str' means use the existing block device with this ID as
>>      backing image
>> 
>> 1.b. of type 'BlockdevOptions' means use the new block device defined by
>>      it as backing image
>> 
>> 1.c. that is null means use no backing image
>>
>> 2. Absent @backing means default to the backing file named in the COW
>>    image.
>
> Over the wire, how you get the difference between 1.c and 2? Are
> you saying that for optional member "backing" we should be
> explicit sending null over the wire?

In the QAPI schema language, absent optional members do not default to
any specific value.  Or in other words, "absent" is distinct from
"present with value V" for any value V.

Now, the *semantics* of "absent" are often identical to some default
value.  Documentation should then say something like (default:
DEFAULT-VALUE).

In this particular instance, it isn't: "absent" means something else
than any possible value.

Aside: no, I don't like this part of the QAPI schema language design
either.  "Absent defaults to DEFAULT-VALUE" is easier to explain and
understand.

Back to your question: to get 1.c., you pass a member "backing": null on
the wire, and to get 2., you pass no "backing" member.

>> Therefore, ...
>>
>> >
>> > Cheers,
>> > Victor
>> >
>> >> > Signed-off-by: Victor Toso <victortoso@redhat.com>
>> >> > ---
>> >> >  qapi/block-core.json | 4 +---
>> >> >  1 file changed, 1 insertion(+), 3 deletions(-)
>> >> >
>> >> > diff --git a/qapi/block-core.json b/qapi/block-core.json
>> >> > index dcc6d41494..302164d575 100644
>> >> > --- a/qapi/block-core.json
>> >> > +++ b/qapi/block-core.json
>> >> > @@ -1542,9 +1542,7 @@
>> >> >  #      "arguments": { "driver": "qcow2",
>> >> >  #                     "node-name": "node1534",
>> >> >  #                     "data-file": { "driver": "file",
>> >> > -#                                    "filename": "hd1.qcow2" },
>> >> > -#                     "backing": null } }
>> >> > -#
>> >> > +#                                    "filename": "hd1.qcow2" } } }
>> >> >  # <- { "return": {} }
>> >> >  #
>> >> >  # -> { "execute": "blockdev-snapshot",
>> >>
>>
>> ... your patch changes the example from 1.c. to 2.  Which is probably
>> not what you intended.
>
> Yep, you are correct but not just with my patch. It is confusing
> that an optional member must be set to JSON Null over the wire..
> I'll need to think a bit more on this.
>
> Many many thanks for your review. Really appreciate it.

You're welcome!

Re: [PATCH v1 16/16] RFC: add a generator for qapi's examples

[Markus Armbruster]

Victor Toso <victortoso@redhat.com> writes:

> Hi,
>
> On Wed, Aug 31, 2022 at 02:01:54PM +0200, Markus Armbruster wrote:
>> Victor Toso <victortoso@redhat.com> writes:
>>
>> > The goal of this generator is to validate QAPI examples and transform
>> > them into a format that can be used for 3rd party applications to
>> > validate their QAPI/QMP introspection.
>> >
>> > For each Example section, we parse server and client messages into a
>> > python dictionary. This step alone has found several ill formatted
>> > JSON messages in the examples.
>> >
>> > The generator outputs another JSON file with all the examples in the
>> > QAPI module that they came from. This can be used to validate the
>> > introspection between QAPI/QMP to language bindings.
>> >
>> > When used with the POC qapi-go branch, we have found bad QMP messages
>> > with wrong member names, mandatory members that were missing and
>> > optional members that were being set with null (not needed).
>> >
>> > A simple example of the output format is:
>> >
>> >  { "examples": [
>> >    {
>> >      "id": "ksuxwzfayw",
>> >      "client": [
>> >      {
>> >        "sequence-order": 1
>> >        "message-type": "command",
>> >        "message":
>> >        { "arguments":
>> >          { "device": "scratch", "size": 1073741824 },
>> >          "execute": "block_resize"
>> >        },
>> >     } ],
>> >     "server": [
>> >     {
>> >       "sequence-order": 2
>> >       "message-type": "return",
>> >       "message": { "return": {} },
>> >     } ]
>> >     }
>> >   ] }
>> >
>> > If this idea seems reasonable, we can add python-qemu-qmp to validate
>> > each message at generation time already.
>> >
>> > Signed-off-by: Victor Toso <victortoso@redhat.com>
>>
>> If I understand you correctly, there are two benefits:
>>
>> 1. Mechanical syntax check for examples
>>
>>    Love it.
>
> Not just JSON syntax but can be extend to the introspection
> layer. Errors like wrong member names would fail while parsing
> the examples (issues such as fixed by patches 11 and 13/16 should
> not happen anymore).

It's also a mechanical check against the schema.  Still love it :)

>> 2. Can extract examples for use as test cases
>>
>>    Sounds good to me.  Possible redundancy with existing tests.
>>    Probably nothing to worry about.
>>
>>    Can you explain in a bit more detail how the extracted data
>>    is (to be) used?
>
> Sure.
>
> The Golang test that consumes this is 152 lines of code [0]. The
> idea is that we can use the examples to feed Golang unmarshalling
> code and then marshall it back to JSON and compare input JSON
> with output JSON and see that their content matches.
>
> [0] https://gitlab.com/victortoso/qapi-go/-/blob/wip-v3/test/examples_test.go
>
> I have generated the examples with this patch series and stored
> the output here [1]
>
> [1] https://gitlab.com/victortoso/qapi-go/-/tree/wip-v3/test/data/examples
>
> The examples are QMP messages that are either sent by Client "->"
> or sent by Server "<-". The order matters so I take the order set
> in the examples and store it as "sequence-order".
>
> In the Go test code, I follow the sequence-order. One example of
> this being useful is that we know which Return type to expect
> after a Command is issued.
>
> I've also included metadata about the type of message, which is
> one of three options: command, event or return. (Errors are
> return too).
>
> This is important because it makes the tests very easy to write.
> Different Unmarshal/Marshal code can be set in the code block of
> the specific message type.
>
> --
>
> The things that makes me quite excited with this idea are:
>
>  1. We have valid functional examples documented. If the examples
>     break, we would have the software in place to know it (plug
>     to ci or some other ninja check seems reasonable to me)
>
>  2. Developers should get more interested in documenting examples
>     as that alone is is a valid test case, even if only useful
>     for language binding's syntax.

Thanks!  Would you like to work some of this into your commit message?

Re: [PATCH v1 12/16] qapi: fix example of blockdev-add command

[Victor Toso]

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

Hi,

First of all, I'm happy that this patch got us into this
discussion.

On Wed, Aug 31, 2022 at 04:53:49PM +0200, Markus Armbruster wrote:
> Victor Toso <victortoso@redhat.com> writes:
>
> > Hi,
> >
> > On Wed, Aug 31, 2022 at 03:16:54PM +0200, Markus Armbruster wrote:
> >> Cc: Kevin for an improved chance of getting any nonsense I might write
> >> corrected.
> >>
> >> Victor Toso <victortoso@redhat.com> writes:
> >>
> >> > Hi,
> >> >
> >> > On Wed, Aug 31, 2022 at 01:40:50PM +0200, Markus Armbruster wrote:
> >> >> Victor Toso <victortoso@redhat.com> writes:
> >> >>
> >> >> > The example output is setting optional member "backing" with null.
> >> >> > This has no runtime impact. Remove it.
> >> >> >
> >> >> > Problem was noticed when using the example as a test case for Go
> >> >> > bindings.
> >> >>
> >> >> "Fix example" and "problem" implies there's something wrong.
> >> >> "No runtime impact" sounds like it's redundant, but not wrong.
> >> >> Wrong or not wrong?
> >> >
> >> > I take your comment is more about the wording which is confusing.
> >> >
> >> > Would it be better if I change to:
> >> > '''
> >> >    The example output is setting optional member "backing" with
> >> >    null. While this has no runtime impact, setting optional
> >> >    members with empty value should not be encouraged. Remove it.
> >> > '''
> >> >
> >> > While I think the above is true, my main reason for proposing
> >> > this change is to re-use the example as a test case, but I'm not
> >> > sure if adding anything related to it would make it better (only
> >> > more confusing!).
> >>
> >> I had a closer look at the schema.
> >>
> >> The definition of backing is
> >>
> >>     ##
> >>     # @BlockdevOptionsGenericCOWFormat:
> >>     #
> >>     # Driver specific block device options for image format that have no option
> >>     # besides their data source and an optional backing file.
> >>     #
> >>     # @backing: reference to or definition of the backing file block
> >>     #           device, null disables the backing file entirely.
> >>     #           Defaults to the backing file stored the image file.
> >>     #
> >>     # Since: 2.9
> >>     ##
> >>     { 'struct': 'BlockdevOptionsGenericCOWFormat',
> >>       'base': 'BlockdevOptionsGenericFormat',
> >>       'data': { '*backing': 'BlockdevRefOrNull' } }
> >>
> >> Meaning, if I remember correctly (with some help from commit
> >> c42e8742f52's message):
> >>
> >> 1. Present @backing
> >>
> >> 1.a. of type 'str' means use the existing block device with this ID as
> >>      backing image
> >>
> >> 1.b. of type 'BlockdevOptions' means use the new block device defined by
> >>      it as backing image
> >>
> >> 1.c. that is null means use no backing image
> >>
> >> 2. Absent @backing means default to the backing file named in the COW
> >>    image.
> >
> > Over the wire, how you get the difference between 1.c and 2? Are
> > you saying that for optional member "backing" we should be
> > explicit sending null over the wire?
>
> In the QAPI schema language, absent optional members do not default to
> any specific value.  Or in other words, "absent" is distinct from
> "present with value V" for any value V.
>
> Now, the *semantics* of "absent" are often identical to some default
> value.  Documentation should then say something like (default:
> DEFAULT-VALUE).

Yep, this is fine.

> In this particular instance, it isn't: "absent" means something else
> than any possible value.

The major painpoint for me is that, in Go an optional member is a
field with a pointer to that field's type. A pointer is default
initialized with nil and if the user of the Go module does
nothing with it, we naturally omit it in the output JSON.

This needs to be workaround in two cases so far:
BlockdevRefOrNull and StrOrNull. This two alternate types are the
only ones that take JSON null as value. I'm sure I'll make it
work.

--

Now, should we really keep using null type as alternative way of
expressing "disabling feature" or even "use something else"?

I'd be happy to work on improving this if that's reasonable. My
2c bellow.

    ##
    # @BlockdevRefOrNull:
    #
    # Reference to a block device.
    #
    # @definition: defines a new block device inline
    # @reference: references the ID of an existing block device.
    #             An empty string means that no block device should
    #             be referenced.  Deprecated; use null instead.
    # @null: No block device should be referenced (since 2.10)
    #
    # Since: 2.9
    ##
    { 'alternate': 'BlockdevRefOrNull',
      'data': { 'definition': 'BlockdevOptions',
                'reference': 'str',
                'null': 'null' } }

BlockdevRefOrNull is only used by BlockdevOptionsGenericCOWFormat
which is used by BlockdevOptions ('qed' and 'vmdk') and extend by
BlockdevOptionsQcow and BlockdevOptionsQcow2.

As you pointed out before, setting backing to null means
disabling. This is expressed in both BlockdevRefOrNull and
@BlockdevOptionsGenericCOWFormat documentation.

IMHO the idea of disabling the default is fine but would be
better expressed with a boolean type, something like:

    { 'alternate': 'BlockdevRefOrNull',
      'data': { 'definition': 'BlockdevOptions',
                'reference': 'str',
                'enable': 'bool' } }

Which makes { backing: false } a bit nicer to my newbie eyes. If
backing member is made non optional, { backing: true } should
have the same value as omitting an optional backing member.

    ##
    # @StrOrNull:
    #
    # This is a string value or the explicit lack of a string (null
    # pointer in C).  Intended for cases when 'optional absent' already
    # has a different meaning.
    #
    # @s: the string value
    # @n: no string value
    #
    # Since: 2.10
    ##
    { 'alternate': 'StrOrNull',
      'data': { 's': 'str',
                'n': 'null' } }

StrOrNull is used in MigrateSetParameters (*tls-creds,
*tls-hostname, *tls-authz):
  JSON null: disable specifics or the entirety of migrating
             with tls.
  "" (empty string): Uses some specifics default.
  omitted: Likely to error if using x509 tls?

  Similarly, a boolean would make more sense to express disabled?

    https://gitlab.com/qemu-project/qemu/-/commit/4af245dc3e6e5c96405b3edb9d75657504256469?view=parallel

StrOrNull is also used in x-blockdev-set-iothread in the iothread
member, documented as follow:

   # @iothread: the name of the IOThread object or null for the
   # main loop

iothread here is non optional, meaning that the user has to set a
string with the name of IOThread object or null. This really
seems a case where iothread could be optional "str" instead of
StrOrNull where omitting defaults to main loop.

> Aside: no, I don't like this part of the QAPI schema language design
> either.  "Absent defaults to DEFAULT-VALUE" is easier to explain and
> understand.

Well, there always room for improving.

> Back to your question: to get 1.c., you pass a member "backing": null on
> the wire, and to get 2., you pass no "backing" member.

Cheers,
Victor

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

Re: [PATCH v1 16/16] RFC: add a generator for qapi's examples

[Victor Toso]

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

Hi,

On Wed, Aug 31, 2022 at 04:57:20PM +0200, Markus Armbruster wrote:
> Victor Toso <victortoso@redhat.com> writes:
> 
> > Hi,
> >
> > On Wed, Aug 31, 2022 at 02:01:54PM +0200, Markus Armbruster wrote:
> >> Victor Toso <victortoso@redhat.com> writes:
> >>
> >> > The goal of this generator is to validate QAPI examples and transform
> >> > them into a format that can be used for 3rd party applications to
> >> > validate their QAPI/QMP introspection.
> >> >
> >> > For each Example section, we parse server and client messages into a
> >> > python dictionary. This step alone has found several ill formatted
> >> > JSON messages in the examples.
> >> >
> >> > The generator outputs another JSON file with all the examples in the
> >> > QAPI module that they came from. This can be used to validate the
> >> > introspection between QAPI/QMP to language bindings.
> >> >
> >> > When used with the POC qapi-go branch, we have found bad QMP messages
> >> > with wrong member names, mandatory members that were missing and
> >> > optional members that were being set with null (not needed).
> >> >
> >> > A simple example of the output format is:
> >> >
> >> >  { "examples": [
> >> >    {
> >> >      "id": "ksuxwzfayw",
> >> >      "client": [
> >> >      {
> >> >        "sequence-order": 1
> >> >        "message-type": "command",
> >> >        "message":
> >> >        { "arguments":
> >> >          { "device": "scratch", "size": 1073741824 },
> >> >          "execute": "block_resize"
> >> >        },
> >> >     } ],
> >> >     "server": [
> >> >     {
> >> >       "sequence-order": 2
> >> >       "message-type": "return",
> >> >       "message": { "return": {} },
> >> >     } ]
> >> >     }
> >> >   ] }
> >> >
> >> > If this idea seems reasonable, we can add python-qemu-qmp to validate
> >> > each message at generation time already.
> >> >
> >> > Signed-off-by: Victor Toso <victortoso@redhat.com>
> >>
> >> If I understand you correctly, there are two benefits:
> >>
> >> 1. Mechanical syntax check for examples
> >>
> >>    Love it.
> >
> > Not just JSON syntax but can be extend to the introspection
> > layer. Errors like wrong member names would fail while parsing
> > the examples (issues such as fixed by patches 11 and 13/16 should
> > not happen anymore).
> 
> It's also a mechanical check against the schema.  Still love it :)

Great :)

> >> 2. Can extract examples for use as test cases
> >>
> >>    Sounds good to me.  Possible redundancy with existing tests.
> >>    Probably nothing to worry about.
> >>
> >>    Can you explain in a bit more detail how the extracted data
> >>    is (to be) used?
> >
> > Sure.
> >
> > The Golang test that consumes this is 152 lines of code [0]. The
> > idea is that we can use the examples to feed Golang unmarshalling
> > code and then marshall it back to JSON and compare input JSON
> > with output JSON and see that their content matches.
> >
> > [0] https://gitlab.com/victortoso/qapi-go/-/blob/wip-v3/test/examples_test.go
> >
> > I have generated the examples with this patch series and stored
> > the output here [1]
> >
> > [1] https://gitlab.com/victortoso/qapi-go/-/tree/wip-v3/test/data/examples
> >
> > The examples are QMP messages that are either sent by Client "->"
> > or sent by Server "<-". The order matters so I take the order set
> > in the examples and store it as "sequence-order".
> >
> > In the Go test code, I follow the sequence-order. One example of
> > this being useful is that we know which Return type to expect
> > after a Command is issued.
> >
> > I've also included metadata about the type of message, which is
> > one of three options: command, event or return. (Errors are
> > return too).
> >
> > This is important because it makes the tests very easy to write.
> > Different Unmarshal/Marshal code can be set in the code block of
> > the specific message type.
> >
> > --
> >
> > The things that makes me quite excited with this idea are:
> >
> >  1. We have valid functional examples documented. If the examples
> >     break, we would have the software in place to know it (plug
> >     to ci or some other ninja check seems reasonable to me)
> >
> >  2. Developers should get more interested in documenting examples
> >     as that alone is is a valid test case, even if only useful
> >     for language binding's syntax.
> 
> Thanks!  Would you like to work some of this into your commit message?

Yeah. I'll resend this series fixing the style you have proposed
and I'll be removing the patches that might need some extra
discussion, like this rfc and examples that are cut short with a
comment.

I'll improve this generator and send it later, probably after the
next iteration of qapi-go. This also gives some room to feedback
from others, if any.

Cheers,
Victor

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

Re: [PATCH v1 12/16] qapi: fix example of blockdev-add command

[Markus Armbruster]

Victor Toso <victortoso@redhat.com> writes:

> Hi,
>
> First of all, I'm happy that this patch got us into this
> discussion.

Me too!

> On Wed, Aug 31, 2022 at 04:53:49PM +0200, Markus Armbruster wrote:
>> Victor Toso <victortoso@redhat.com> writes:
>>
>> > Hi,
>> >
>> > On Wed, Aug 31, 2022 at 03:16:54PM +0200, Markus Armbruster wrote:
>> >> Cc: Kevin for an improved chance of getting any nonsense I might write
>> >> corrected.
>> >>
>> >> Victor Toso <victortoso@redhat.com> writes:
>> >>
>> >> > Hi,
>> >> >
>> >> > On Wed, Aug 31, 2022 at 01:40:50PM +0200, Markus Armbruster wrote:
>> >> >> Victor Toso <victortoso@redhat.com> writes:
>> >> >>
>> >> >> > The example output is setting optional member "backing" with null.
>> >> >> > This has no runtime impact. Remove it.
>> >> >> >
>> >> >> > Problem was noticed when using the example as a test case for Go
>> >> >> > bindings.
>> >> >>
>> >> >> "Fix example" and "problem" implies there's something wrong.
>> >> >> "No runtime impact" sounds like it's redundant, but not wrong.
>> >> >> Wrong or not wrong?
>> >> >
>> >> > I take your comment is more about the wording which is confusing.
>> >> >
>> >> > Would it be better if I change to:
>> >> > '''
>> >> >    The example output is setting optional member "backing" with
>> >> >    null. While this has no runtime impact, setting optional
>> >> >    members with empty value should not be encouraged. Remove it.
>> >> > '''
>> >> >
>> >> > While I think the above is true, my main reason for proposing
>> >> > this change is to re-use the example as a test case, but I'm not
>> >> > sure if adding anything related to it would make it better (only
>> >> > more confusing!).
>> >>
>> >> I had a closer look at the schema.
>> >>
>> >> The definition of backing is
>> >>
>> >>     ##
>> >>     # @BlockdevOptionsGenericCOWFormat:
>> >>     #
>> >>     # Driver specific block device options for image format that have no option
>> >>     # besides their data source and an optional backing file.
>> >>     #
>> >>     # @backing: reference to or definition of the backing file block
>> >>     #           device, null disables the backing file entirely.
>> >>     #           Defaults to the backing file stored the image file.
>> >>     #
>> >>     # Since: 2.9
>> >>     ##
>> >>     { 'struct': 'BlockdevOptionsGenericCOWFormat',
>> >>       'base': 'BlockdevOptionsGenericFormat',
>> >>       'data': { '*backing': 'BlockdevRefOrNull' } }
>> >>
>> >> Meaning, if I remember correctly (with some help from commit
>> >> c42e8742f52's message):
>> >>
>> >> 1. Present @backing
>> >>
>> >> 1.a. of type 'str' means use the existing block device with this ID as
>> >>      backing image
>> >>
>> >> 1.b. of type 'BlockdevOptions' means use the new block device defined by
>> >>      it as backing image
>> >>
>> >> 1.c. that is null means use no backing image
>> >>
>> >> 2. Absent @backing means default to the backing file named in the COW
>> >>    image.
>> >
>> > Over the wire, how you get the difference between 1.c and 2? Are
>> > you saying that for optional member "backing" we should be
>> > explicit sending null over the wire?
>>
>> In the QAPI schema language, absent optional members do not default to
>> any specific value.  Or in other words, "absent" is distinct from
>> "present with value V" for any value V.
>>
>> Now, the *semantics* of "absent" are often identical to some default
>> value.  Documentation should then say something like (default:
>> DEFAULT-VALUE).
>
> Yep, this is fine.
>
>> In this particular instance, it isn't: "absent" means something else
>> than any possible value.
>
> The major painpoint for me is that, in Go an optional member is a
> field with a pointer to that field's type. A pointer is default
> initialized with nil and if the user of the Go module does
> nothing with it, we naturally omit it in the output JSON.
>
> This needs to be workaround in two cases so far:
> BlockdevRefOrNull and StrOrNull. This two alternate types are the
> only ones that take JSON null as value. I'm sure I'll make it
> work.

Losely related:

commit 481b002cc81ed7fc7b06e32e9d4d495d81739d14
Author: Markus Armbruster <armbru@redhat.com>
Date:   Wed Apr 29 15:35:05 2015 -0600

    qobject: Add a special null QObject
    
    I'm going to fix the JSON parser to recognize null.  The obvious
    representation of JSON null as (QObject *)NULL doesn't work, because
    the parser already uses it as an error value.  Perhaps we should
    change it to free NULL for null, but that's more than I can do right
    now.  Create a special null QObject instead.
    
    The existing QDict, QList, and QString all represent something that
    is a pointer in C and could therefore be associated with NULL.  But
    right now, all three of these sub-types are always non-null once
    created, so the new null sentinel object is intentionally unrelated
    to them.
    
    Signed-off-by: Markus Armbruster <armbru@redhat.com>
    Signed-off-by: Eric Blake <eblake@redhat.com>
    Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>

I figure something like this should work for Go as well, i.e. create a
singleton type to represent the QAPI type 'null'.

> --
>
> Now, should we really keep using null type as alternative way of
> expressing "disabling feature" or even "use something else"?
>
> I'd be happy to work on improving this if that's reasonable. My
> 2c bellow.
>
>     ##
>     # @BlockdevRefOrNull:
>     #
>     # Reference to a block device.
>     #
>     # @definition: defines a new block device inline
>     # @reference: references the ID of an existing block device.
>     #             An empty string means that no block device should
>     #             be referenced.  Deprecated; use null instead.
>     # @null: No block device should be referenced (since 2.10)
>     #
>     # Since: 2.9
>     ##
>     { 'alternate': 'BlockdevRefOrNull',
>       'data': { 'definition': 'BlockdevOptions',
>                 'reference': 'str',
>                 'null': 'null' } }
>
> BlockdevRefOrNull is only used by BlockdevOptionsGenericCOWFormat
> which is used by BlockdevOptions ('qed' and 'vmdk') and extend by
> BlockdevOptionsQcow and BlockdevOptionsQcow2.
>
> As you pointed out before, setting backing to null means
> disabling. This is expressed in both BlockdevRefOrNull and
> @BlockdevOptionsGenericCOWFormat documentation.
>
> IMHO the idea of disabling the default is fine but would be
> better expressed with a boolean type, something like:
>
>     { 'alternate': 'BlockdevRefOrNull',
>       'data': { 'definition': 'BlockdevOptions',
>                 'reference': 'str',
>                 'enable': 'bool' } }
>
> Which makes { backing: false } a bit nicer to my newbie eyes. If
> backing member is made non optional, { backing: true } should
> have the same value as omitting an optional backing member.

So:

1. Present @backing

1.a. of type 'str' means use the existing block device with this ID as
     backing image

1.b. of type 'BlockdevOptions' means use the new block device defined by
     it as backing image

1.c. that is false means use no backing image

1.d. that is true means use the backing file named in the COW
     image.

2. Absent @backing defaults to true, i.e. 1.d.

Looks fine to me as an interface, if I ignore compatibility concerns.

We could quibble about the name 'enable'.  It's not part of the external
interface.

Sadly, the idea runs afoul a QAPI restriction:

    scripts/qapi-gen.py: In file included from ../qapi/qapi-schema.json:68:
    In file included from ../qapi/block.json:8:
    ../qapi/block-core.json: In alternate 'BlockdevRefOrNull':
    ../qapi/block-core.json:4362: branch 'enabled' can't be distinguished from 'reference'

This is due to

commit c0644771ebedbd8f47f3c24816445e30111d226b (tag: pull-qapi-2017-05-31)
Author: Markus Armbruster <armbru@redhat.com>
Date:   Mon May 22 18:42:15 2017 +0200

    qapi: Reject alternates that can't work with keyval_parse()
    
    Alternates are sum types like unions, but use the JSON type on the
    wire / QType in QObject instead of an explicit tag.  That's why we
    require alternate members to have distinct QTypes.
    
    The recently introduced keyval_parse() (commit d454dbe) can only
    produce string scalars.  The qobject_input_visitor_new_keyval() input
    visitor mostly hides the difference, so code using a QObject input
    visitor doesn't have to care whether its input was parsed from JSON or
    KEY=VALUE,...  The difference leaks for alternates, as noted in commit
    0ee9ae7: a non-string, non-enum scalar alternate value can't currently
    be expressed.
    
    In part, this is just our insufficiently sophisticated implementation.
    Consider alternate type 'GuestFileWhence'.  It has an integer member
    and a 'QGASeek' member.  The latter is an enumeration with values
    'set', 'cur', 'end'.  The meaning of b=set, b=cur, b=end, b=0, b=1 and
    so forth is perfectly obvious.  However, our current implementation
    falls apart at run time for b=0, b=1, and so forth.  Fixable, but not
    today; add a test case and a TODO comment.
    
    Now consider an alternate type with a string and an integer member.
    What's the meaning of a=42?  Is it the string "42" or the integer 42?
    Whichever meaning you pick makes the other inexpressible.  This isn't
    just an implementation problem, it's fundamental.  Our current
    implementation will pick string.
    
    So far, we haven't needed such alternates.  To make sure we stop and
    think before we add one that cannot sanely work with keyval_parse(),
    let's require alternate members to have sufficiently distinct
    representation in KEY=VALUE,... syntax:
    
    * A string member clashes with any other scalar member
    
    * An enumeration member clashes with bool members when it has value
      'on' or 'off'.
    
    * An enumeration member clashes with numeric members when it has a
      value that starts with '-', '+', or a decimal digit.  This is a
      rather lazy approximation of the actual number syntax accepted by
      the visitor.
    
      Note that enumeration values starting with '-' and '+' are rejected
      elsewhere already, but better safe than sorry.
    
    Signed-off-by: Markus Armbruster <armbru@redhat.com>
    Message-Id: <1495471335-23707-5-git-send-email-armbru@redhat.com>
    Reviewed-by: Eric Blake <eblake@redhat.com>
    Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Astute readers may now wonder why our string member does *not* clash
with scalar member null.  The answer is kind of embarrassing: because
the keyval input visitor supports numbers and booleans, but not null.
If you need null, you get to use JSON.

If we enhanced the visitor to support null like it supports booleans,
the value null would become ambiguous, and to remove this ambiguity,
we'd have to make 'str' clash with 'null'.

I've come to regret adding 'null' to the QAPI schema language.  I didn't
fully understand how this innocent-looking feature was going to interact
with everything else.  More trouble than it's worth.

The dotted keys syntax as an alternative to JSON is also plenty of
trouble, but we knew that, and accepted it because there's also worth.

>     ##
>     # @StrOrNull:
>     #
>     # This is a string value or the explicit lack of a string (null
>     # pointer in C).  Intended for cases when 'optional absent' already
>     # has a different meaning.
>     #
>     # @s: the string value
>     # @n: no string value
>     #
>     # Since: 2.10
>     ##
>     { 'alternate': 'StrOrNull',
>       'data': { 's': 'str',
>                 'n': 'null' } }
>
> StrOrNull is used in MigrateSetParameters (*tls-creds,
> *tls-hostname, *tls-authz):
>   JSON null: disable specifics or the entirety of migrating
>              with tls.
>   "" (empty string): Uses some specifics default.
>   omitted: Likely to error if using x509 tls?
>
>   Similarly, a boolean would make more sense to express disabled?
>
>     https://gitlab.com/qemu-project/qemu/-/commit/4af245dc3e6e5c96405b3edb9d75657504256469?view=parallel
>
> StrOrNull is also used in x-blockdev-set-iothread in the iothread
> member, documented as follow:
>
>    # @iothread: the name of the IOThread object or null for the
>    # main loop
>
> iothread here is non optional, meaning that the user has to set a
> string with the name of IOThread object or null. This really
> seems a case where iothread could be optional "str" instead of
> StrOrNull where omitting defaults to main loop.

Then "absent" is not the same as "present with value V" for any value V.
That's okay; it conforms to the schema language design.  I just don't
like that part of the schema language design.  To be clear: insufficient
reason to reject a patch.

We'd have to provide backward compatibility somehow.

>> Aside: no, I don't like this part of the QAPI schema language design
>> either.  "Absent defaults to DEFAULT-VALUE" is easier to explain and
>> understand.
>
> Well, there always room for improving.

Here's a concrete improvement I'd like to see some day: extend the QAPI
language to support optionals with a default value, i.e. absent is
indistinguishable from present with the default value.  Then use this
for as many optionals as we can.

>> Back to your question: to get 1.c., you pass a member "backing": null on
>> the wire, and to get 2., you pass no "backing" member.
>
> Cheers,
> Victor

Re: [PATCH v1 03/16] qapi: fix example of query-spice command

[Gerd Hoffmann]

  Hi,

> I agree listing more channels in the example wouldn't help users.
> 
> But do we want to indicate that the example is abridged?

Hard to do if the result should be valid qmp ...

Maybe add a second channel to clearly show the command returns
a list of channels?

I'm also curious why you check the syntax in the first place.
Make sure syntax highlight in the documentation works properly?
Make sure the examples are not outdated?

> Gerd, I'd like to have your Acked-by for this patch.

I don't mind that much.  If there is a good reason go for it.

Acked-by: Gerd Hoffmann <kraxel@redhat.com>

take care,
  Gerd

Re: [PATCH v1 12/16] qapi: fix example of blockdev-add command

[Victor Toso]

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

Hi,

On Thu, Sep 01, 2022 at 01:13:19PM +0200, Markus Armbruster wrote:
> Victor Toso <victortoso@redhat.com> writes:
> 
> > Hi,
> >
> > First of all, I'm happy that this patch got us into this
> > discussion.
> 
> Me too!
> 
> > On Wed, Aug 31, 2022 at 04:53:49PM +0200, Markus Armbruster wrote:
> >> Victor Toso <victortoso@redhat.com> writes:
> >>
> >> > Hi,
> >> >
> >> > On Wed, Aug 31, 2022 at 03:16:54PM +0200, Markus Armbruster wrote:
> >> >> Cc: Kevin for an improved chance of getting any nonsense I might write
> >> >> corrected.
> >> >>
> >> >> Victor Toso <victortoso@redhat.com> writes:
> >> >>
> >> >> > Hi,
> >> >> >
> >> >> > On Wed, Aug 31, 2022 at 01:40:50PM +0200, Markus Armbruster wrote:
> >> >> >> Victor Toso <victortoso@redhat.com> writes:
> >> >> >>
> >> >> >> > The example output is setting optional member "backing" with null.
> >> >> >> > This has no runtime impact. Remove it.
> >> >> >> >
> >> >> >> > Problem was noticed when using the example as a test case for Go
> >> >> >> > bindings.
> >> >> >>
> >> >> >> "Fix example" and "problem" implies there's something wrong.
> >> >> >> "No runtime impact" sounds like it's redundant, but not wrong.
> >> >> >> Wrong or not wrong?
> >> >> >
> >> >> > I take your comment is more about the wording which is confusing.
> >> >> >
> >> >> > Would it be better if I change to:
> >> >> > '''
> >> >> >    The example output is setting optional member "backing" with
> >> >> >    null. While this has no runtime impact, setting optional
> >> >> >    members with empty value should not be encouraged. Remove it.
> >> >> > '''
> >> >> >
> >> >> > While I think the above is true, my main reason for proposing
> >> >> > this change is to re-use the example as a test case, but I'm not
> >> >> > sure if adding anything related to it would make it better (only
> >> >> > more confusing!).
> >> >>
> >> >> I had a closer look at the schema.
> >> >>
> >> >> The definition of backing is
> >> >>
> >> >>     ##
> >> >>     # @BlockdevOptionsGenericCOWFormat:
> >> >>     #
> >> >>     # Driver specific block device options for image format that have no option
> >> >>     # besides their data source and an optional backing file.
> >> >>     #
> >> >>     # @backing: reference to or definition of the backing file block
> >> >>     #           device, null disables the backing file entirely.
> >> >>     #           Defaults to the backing file stored the image file.
> >> >>     #
> >> >>     # Since: 2.9
> >> >>     ##
> >> >>     { 'struct': 'BlockdevOptionsGenericCOWFormat',
> >> >>       'base': 'BlockdevOptionsGenericFormat',
> >> >>       'data': { '*backing': 'BlockdevRefOrNull' } }
> >> >>
> >> >> Meaning, if I remember correctly (with some help from commit
> >> >> c42e8742f52's message):
> >> >>
> >> >> 1. Present @backing
> >> >>
> >> >> 1.a. of type 'str' means use the existing block device with this ID as
> >> >>      backing image
> >> >>
> >> >> 1.b. of type 'BlockdevOptions' means use the new block device defined by
> >> >>      it as backing image
> >> >>
> >> >> 1.c. that is null means use no backing image
> >> >>
> >> >> 2. Absent @backing means default to the backing file named in the COW
> >> >>    image.
> >> >
> >> > Over the wire, how you get the difference between 1.c and 2? Are
> >> > you saying that for optional member "backing" we should be
> >> > explicit sending null over the wire?
> >>
> >> In the QAPI schema language, absent optional members do not default to
> >> any specific value.  Or in other words, "absent" is distinct from
> >> "present with value V" for any value V.
> >>
> >> Now, the *semantics* of "absent" are often identical to some default
> >> value.  Documentation should then say something like (default:
> >> DEFAULT-VALUE).
> >
> > Yep, this is fine.
> >
> >> In this particular instance, it isn't: "absent" means something else
> >> than any possible value.
> >
> > The major painpoint for me is that, in Go an optional member is a
> > field with a pointer to that field's type. A pointer is default
> > initialized with nil and if the user of the Go module does
> > nothing with it, we naturally omit it in the output JSON.
> >
> > This needs to be workaround in two cases so far:
> > BlockdevRefOrNull and StrOrNull. This two alternate types are the
> > only ones that take JSON null as value. I'm sure I'll make it
> > work.
> 
> Losely related:
> 
> commit 481b002cc81ed7fc7b06e32e9d4d495d81739d14
> Author: Markus Armbruster <armbru@redhat.com>
> Date:   Wed Apr 29 15:35:05 2015 -0600
> 
>     qobject: Add a special null QObject
>     
>     I'm going to fix the JSON parser to recognize null.  The obvious
>     representation of JSON null as (QObject *)NULL doesn't work, because
>     the parser already uses it as an error value.  Perhaps we should
>     change it to free NULL for null, but that's more than I can do right
>     now.  Create a special null QObject instead.
>     
>     The existing QDict, QList, and QString all represent something that
>     is a pointer in C and could therefore be associated with NULL.  But
>     right now, all three of these sub-types are always non-null once
>     created, so the new null sentinel object is intentionally unrelated
>     to them.
>     
>     Signed-off-by: Markus Armbruster <armbru@redhat.com>
>     Signed-off-by: Eric Blake <eblake@redhat.com>
>     Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> 
> I figure something like this should work for Go as well, i.e. create a
> singleton type to represent the QAPI type 'null'.
> 
> > --
> >
> > Now, should we really keep using null type as alternative way of
> > expressing "disabling feature" or even "use something else"?
> >
> > I'd be happy to work on improving this if that's reasonable. My
> > 2c bellow.
> >
> >     ##
> >     # @BlockdevRefOrNull:
> >     #
> >     # Reference to a block device.
> >     #
> >     # @definition: defines a new block device inline
> >     # @reference: references the ID of an existing block device.
> >     #             An empty string means that no block device should
> >     #             be referenced.  Deprecated; use null instead.
> >     # @null: No block device should be referenced (since 2.10)
> >     #
> >     # Since: 2.9
> >     ##
> >     { 'alternate': 'BlockdevRefOrNull',
> >       'data': { 'definition': 'BlockdevOptions',
> >                 'reference': 'str',
> >                 'null': 'null' } }
> >
> > BlockdevRefOrNull is only used by BlockdevOptionsGenericCOWFormat
> > which is used by BlockdevOptions ('qed' and 'vmdk') and extend by
> > BlockdevOptionsQcow and BlockdevOptionsQcow2.
> >
> > As you pointed out before, setting backing to null means
> > disabling. This is expressed in both BlockdevRefOrNull and
> > @BlockdevOptionsGenericCOWFormat documentation.
> >
> > IMHO the idea of disabling the default is fine but would be
> > better expressed with a boolean type, something like:
> >
> >     { 'alternate': 'BlockdevRefOrNull',
> >       'data': { 'definition': 'BlockdevOptions',
> >                 'reference': 'str',
> >                 'enable': 'bool' } }
> >
> > Which makes { backing: false } a bit nicer to my newbie eyes. If
> > backing member is made non optional, { backing: true } should
> > have the same value as omitting an optional backing member.
> 
> So:
> 
> 1. Present @backing
> 
> 1.a. of type 'str' means use the existing block device with this ID as
>      backing image
> 
> 1.b. of type 'BlockdevOptions' means use the new block device defined by
>      it as backing image
> 
> 1.c. that is false means use no backing image
> 
> 1.d. that is true means use the backing file named in the COW
>      image.
> 
> 2. Absent @backing defaults to true, i.e. 1.d.
> 
> Looks fine to me as an interface, if I ignore compatibility concerns.
> 
> We could quibble about the name 'enable'.  It's not part of the external
> interface.
> 
> Sadly, the idea runs afoul a QAPI restriction:
> 
>     scripts/qapi-gen.py: In file included from ../qapi/qapi-schema.json:68:
>     In file included from ../qapi/block.json:8:
>     ../qapi/block-core.json: In alternate 'BlockdevRefOrNull':
>     ../qapi/block-core.json:4362: branch 'enabled' can't be distinguished from 'reference'
> 
> This is due to
> 
> commit c0644771ebedbd8f47f3c24816445e30111d226b (tag: pull-qapi-2017-05-31)
> Author: Markus Armbruster <armbru@redhat.com>
> Date:   Mon May 22 18:42:15 2017 +0200
> 
>     qapi: Reject alternates that can't work with keyval_parse()
>     
>     Alternates are sum types like unions, but use the JSON type on the
>     wire / QType in QObject instead of an explicit tag.  That's why we
>     require alternate members to have distinct QTypes.
>     
>     The recently introduced keyval_parse() (commit d454dbe) can only
>     produce string scalars.  The qobject_input_visitor_new_keyval() input
>     visitor mostly hides the difference, so code using a QObject input
>     visitor doesn't have to care whether its input was parsed from JSON or
>     KEY=VALUE,...  The difference leaks for alternates, as noted in commit
>     0ee9ae7: a non-string, non-enum scalar alternate value can't currently
>     be expressed.
>     
>     In part, this is just our insufficiently sophisticated implementation.
>     Consider alternate type 'GuestFileWhence'.  It has an integer member
>     and a 'QGASeek' member.  The latter is an enumeration with values
>     'set', 'cur', 'end'.  The meaning of b=set, b=cur, b=end, b=0, b=1 and
>     so forth is perfectly obvious.  However, our current implementation
>     falls apart at run time for b=0, b=1, and so forth.  Fixable, but not
>     today; add a test case and a TODO comment.
>     
>     Now consider an alternate type with a string and an integer member.
>     What's the meaning of a=42?  Is it the string "42" or the integer 42?
>     Whichever meaning you pick makes the other inexpressible.  This isn't
>     just an implementation problem, it's fundamental.  Our current
>     implementation will pick string.
>     
>     So far, we haven't needed such alternates.  To make sure we stop and
>     think before we add one that cannot sanely work with keyval_parse(),
>     let's require alternate members to have sufficiently distinct
>     representation in KEY=VALUE,... syntax:
>     
>     * A string member clashes with any other scalar member
>     
>     * An enumeration member clashes with bool members when it has value
>       'on' or 'off'.
>     
>     * An enumeration member clashes with numeric members when it has a
>       value that starts with '-', '+', or a decimal digit.  This is a
>       rather lazy approximation of the actual number syntax accepted by
>       the visitor.
>     
>       Note that enumeration values starting with '-' and '+' are rejected
>       elsewhere already, but better safe than sorry.
>     
>     Signed-off-by: Markus Armbruster <armbru@redhat.com>
>     Message-Id: <1495471335-23707-5-git-send-email-armbru@redhat.com>
>     Reviewed-by: Eric Blake <eblake@redhat.com>
>     Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Astute readers may now wonder why our string member does *not* clash
> with scalar member null.  The answer is kind of embarrassing: because
> the keyval input visitor supports numbers and booleans, but not null.
> If you need null, you get to use JSON.
> 
> If we enhanced the visitor to support null like it supports booleans,
> the value null would become ambiguous, and to remove this ambiguity,
> we'd have to make 'str' clash with 'null'.
> 
> I've come to regret adding 'null' to the QAPI schema language.  I didn't
> fully understand how this innocent-looking feature was going to interact
> with everything else.  More trouble than it's worth.
> 
> The dotted keys syntax as an alternative to JSON is also plenty of
> trouble, but we knew that, and accepted it because there's also worth.
> 
> >     ##
> >     # @StrOrNull:
> >     #
> >     # This is a string value or the explicit lack of a string (null
> >     # pointer in C).  Intended for cases when 'optional absent' already
> >     # has a different meaning.
> >     #
> >     # @s: the string value
> >     # @n: no string value
> >     #
> >     # Since: 2.10
> >     ##
> >     { 'alternate': 'StrOrNull',
> >       'data': { 's': 'str',
> >                 'n': 'null' } }
> >
> > StrOrNull is used in MigrateSetParameters (*tls-creds,
> > *tls-hostname, *tls-authz):
> >   JSON null: disable specifics or the entirety of migrating
> >              with tls.
> >   "" (empty string): Uses some specifics default.
> >   omitted: Likely to error if using x509 tls?
> >
> >   Similarly, a boolean would make more sense to express disabled?
> >
> >     https://gitlab.com/qemu-project/qemu/-/commit/4af245dc3e6e5c96405b3edb9d75657504256469?view=parallel
> >
> > StrOrNull is also used in x-blockdev-set-iothread in the iothread
> > member, documented as follow:
> >
> >    # @iothread: the name of the IOThread object or null for the
> >    # main loop
> >
> > iothread here is non optional, meaning that the user has to set a
> > string with the name of IOThread object or null. This really
> > seems a case where iothread could be optional "str" instead of
> > StrOrNull where omitting defaults to main loop.
> 
> Then "absent" is not the same as "present with value V" for any value V.
> That's okay; it conforms to the schema language design.  I just don't
> like that part of the schema language design.  To be clear: insufficient
> reason to reject a patch.
> 
> We'd have to provide backward compatibility somehow.
> 
> >> Aside: no, I don't like this part of the QAPI schema language design
> >> either.  "Absent defaults to DEFAULT-VALUE" is easier to explain and
> >> understand.
> >
> > Well, there always room for improving.
> 
> Here's a concrete improvement I'd like to see some day: extend the QAPI
> language to support optionals with a default value, i.e. absent is
> indistinguishable from present with the default value.  Then use this
> for as many optionals as we can.

This would be nice indeed.  I've filed an issue in Gitlab to
track it:

    https://gitlab.com/qemu-project/qemu/-/issues/1188

Cheers,
Victor

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