[PATCH v1 0/7] Validate and test qapi examples

[PATCH v1 0/7] Validate and test qapi examples

[Victor Toso]

Hi,

This is a follow up from the RFC sent in the end of 08-2022:
    https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg04525.html

The generator code was rebased, without conflicts. The commit log was
improved as per Markus suggestion [0], altough I'm sure it can be
improved further.

To clarify, consuming the Examples as data for testing the qapi-go
work has been very very helpful. I'm positive it can be of use for other
bindings in the future, besides keeping the examples functional.

Cheers,

[0] https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg04682.html

Victor Toso (7):
  qapi: scripts: add a generator for qapi's examples
  qapi: fix example of get-win32-socket command
  qapi: fix example of dumpdtb command
  qapi: fix example of cancel-vcpu-dirty-limit command
  qapi: fix example of set-vcpu-dirty-limit command
  qapi: fix example of calc-dirty-rate command
  qapi: fix example of NETDEV_STREAM_CONNECTED event

 qapi/machine.json            |   2 +-
 qapi/migration.json          |   6 +-
 qapi/misc.json               |   2 +-
 qapi/net.json                |   6 +-
 scripts/qapi/dumpexamples.py | 194 +++++++++++++++++++++++++++++++++++
 scripts/qapi/main.py         |   2 +
 6 files changed, 204 insertions(+), 8 deletions(-)
 create mode 100644 scripts/qapi/dumpexamples.py

-- 
2.41.0

[PATCH v1 1/7] qapi: scripts: add a generator for qapi's examples

[Victor Toso]

This generator has two goals:
 1. Mechanical validation of QAPI examples
 2. Generate the examples in a JSON format to be consumed for extra
    validation.

The generator iterates over every Example section, parsing both server
and client messages. The generator prints any inconsistency found, for
example:

 |  Error: Extra data: line 1 column 39 (char 38)
 |  Location: cancel-vcpu-dirty-limit at qapi/migration.json:2017
 |  Data: {"execute": "cancel-vcpu-dirty-limit"},
 |      "arguments": { "cpu-index": 1 } }

The generator will output other 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, for example:

 | { "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": {} },
 |    } ]
 |    }
 |  ] }

Note that the order matters, as read by the Example section and
translated into "sequence-order". A language binding project can then
consume this files to Marshal and Unmarshal, comparing if the results
are what is to be expected.

RFC discussion:
    https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg04641.html

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 316736b6a2..cf9beac3c9 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.41.0

[PATCH v1 2/7] qapi: fix example of get-win32-socket command

[Victor Toso]

Example output lacks double quotes. Fix it.

Fixes: 4cda177c60 "qmp: add 'get-win32-socket'"
Signed-off-by: Victor Toso <victortoso@redhat.com>
---
 qapi/misc.json | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qapi/misc.json b/qapi/misc.json
index cda2effa81..be302cadeb 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -290,7 +290,7 @@
 #
 # Example:
 #
-# -> { "execute": "get-win32-socket", "arguments": { "info": "abcd123..", fdname": "skclient" } }
+# -> { "execute": "get-win32-socket", "arguments": { "info": "abcd123..", "fdname": "skclient" } }
 # <- { "return": {} }
 ##
 { 'command': 'get-win32-socket', 'data': {'info': 'str', 'fdname': 'str'}, 'if': 'CONFIG_WIN32' }
-- 
2.41.0

[PATCH v1 3/7] qapi: fix example of dumpdtb command

[Victor Toso]

Example output has extra end curly bracket. Switch with comma.

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 a08b6576ca..9eb76193e0 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1684,7 +1684,7 @@
 #
 # Example:
 #
-# -> { "execute": "dumpdtb" }
+# -> { "execute": "dumpdtb",
 #      "arguments": { "filename": "fdt.dtb" } }
 # <- { "return": {} }
 ##
-- 
2.41.0

[PATCH v1 4/7] qapi: fix example of cancel-vcpu-dirty-limit command

[Victor Toso]

Example output has extra end curly bracket. Remove it.

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

diff --git a/qapi/migration.json b/qapi/migration.json
index 8843e74b59..9385b9f87c 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -2010,7 +2010,7 @@
 #
 # Example:
 #
-# -> {"execute": "cancel-vcpu-dirty-limit"},
+# -> {"execute": "cancel-vcpu-dirty-limit",
 #     "arguments": { "cpu-index": 1 } }
 # <- { "return": {} }
 ##
-- 
2.41.0

[PATCH v1 5/7] qapi: fix example of set-vcpu-dirty-limit command

[Victor Toso]

Example output has extra end curly bracket. Remove it.

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

diff --git a/qapi/migration.json b/qapi/migration.json
index 9385b9f87c..2658cdbcbe 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1986,7 +1986,7 @@
 #
 # Example:
 #
-# -> {"execute": "set-vcpu-dirty-limit"}
+# -> {"execute": "set-vcpu-dirty-limit",
 #     "arguments": { "dirty-rate": 200,
 #                    "cpu-index": 1 } }
 # <- { "return": {} }
-- 
2.41.0

[PATCH v1 6/7] qapi: fix example of calc-dirty-rate command

[Victor Toso]

Example output has property name with single quotes. Fix it.

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

diff --git a/qapi/migration.json b/qapi/migration.json
index 2658cdbcbe..45dac41f67 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1922,7 +1922,7 @@
 # Example:
 #
 # -> {"execute": "calc-dirty-rate", "arguments": {"calc-time": 1,
-#                                                 'sample-pages': 512} }
+#                                                 "sample-pages": 512} }
 # <- { "return": {} }
 ##
 { 'command': 'calc-dirty-rate', 'data': {'calc-time': 'int64',
-- 
2.41.0

[PATCH v1 7/7] qapi: fix example of NETDEV_STREAM_CONNECTED event

[Victor Toso]

Example output was using single quotes. Fix it.

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

diff --git a/qapi/net.json b/qapi/net.json
index 313c8a606e..81988e499a 100644
--- a/qapi/net.json
+++ b/qapi/net.json
@@ -930,9 +930,9 @@
 #
 # Example:
 #
-# <- { 'event': 'NETDEV_STREAM_DISCONNECTED',
-#      'data': {'netdev-id': 'netdev0'},
-#      'timestamp': {'seconds': 1663330937, 'microseconds': 526695} }
+# <- { "event": "NETDEV_STREAM_DISCONNECTED",
+#      "data": {"netdev-id": "netdev0"},
+#      "timestamp": {"seconds": 1663330937, "microseconds": 526695} }
 ##
 { 'event': 'NETDEV_STREAM_DISCONNECTED',
   'data': { 'netdev-id': 'str' } }
-- 
2.41.0

Re: [PATCH v1 7/7] qapi: fix example of NETDEV_STREAM_CONNECTED event

[Daniel P. Berrangé]

On Tue, Sep 05, 2023 at 09:48:46PM +0200, Victor Toso wrote:
> Example output was using single quotes. Fix it.
> 
> Signed-off-by: Victor Toso <victortoso@redhat.com>
> ---
>  qapi/net.json | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v1 6/7] qapi: fix example of calc-dirty-rate command

[Daniel P. Berrangé]

On Tue, Sep 05, 2023 at 09:48:45PM +0200, Victor Toso wrote:
> Example output has property name with single quotes. Fix it.
> 
> Signed-off-by: Victor Toso <victortoso@redhat.com>
> ---
>  qapi/migration.json | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v1 5/7] qapi: fix example of set-vcpu-dirty-limit command

[Daniel P. Berrangé]

On Tue, Sep 05, 2023 at 09:48:44PM +0200, Victor Toso wrote:
> Example output has extra end curly bracket. Remove it.
> 
> Signed-off-by: Victor Toso <victortoso@redhat.com>
> ---
>  qapi/migration.json | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v1 4/7] qapi: fix example of cancel-vcpu-dirty-limit command

[Daniel P. Berrangé]

On Tue, Sep 05, 2023 at 09:48:43PM +0200, Victor Toso wrote:
> Example output has extra end curly bracket. Remove it.
> 
> Signed-off-by: Victor Toso <victortoso@redhat.com>
> ---
>  qapi/migration.json | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v1 3/7] qapi: fix example of dumpdtb command

[Daniel P. Berrangé]

On Tue, Sep 05, 2023 at 09:48:42PM +0200, Victor Toso wrote:
> Example output has extra end curly bracket. Switch with comma.
> 
> Signed-off-by: Victor Toso <victortoso@redhat.com>
> ---
>  qapi/machine.json | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v1 2/7] qapi: fix example of get-win32-socket command

[Daniel P. Berrangé]

On Tue, Sep 05, 2023 at 09:48:41PM +0200, Victor Toso wrote:
> Example output lacks double quotes. Fix it.
> 
> Fixes: 4cda177c60 "qmp: add 'get-win32-socket'"
> Signed-off-by: Victor Toso <victortoso@redhat.com>
> ---
>  qapi/misc.json | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v1 1/7] qapi: scripts: add a generator for qapi's examples

[Daniel P. Berrangé]

On Tue, Sep 05, 2023 at 09:48:40PM +0200, Victor Toso wrote:
> This generator has two goals:
>  1. Mechanical validation of QAPI examples
>  2. Generate the examples in a JSON format to be consumed for extra
>     validation.
> 
> The generator iterates over every Example section, parsing both server
> and client messages. The generator prints any inconsistency found, for
> example:
> 
>  |  Error: Extra data: line 1 column 39 (char 38)
>  |  Location: cancel-vcpu-dirty-limit at qapi/migration.json:2017
>  |  Data: {"execute": "cancel-vcpu-dirty-limit"},
>  |      "arguments": { "cpu-index": 1 } }
> 
> The generator will output other 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, for example:
> 
>  | { "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": {} },
>  |    } ]
>  |    }
>  |  ] }
> 
> Note that the order matters, as read by the Example section and
> translated into "sequence-order". A language binding project can then
> consume this files to Marshal and Unmarshal, comparing if the results
> are what is to be expected.
> 
> RFC discussion:
>     https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg04641.html
> 
> 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]))

This prints an error, but the caller ignores this and carries on
as normal.

After applying this series, we still have multiple errors being
printed on console

Error: Expecting ',' delimiter: line 12 column 19 (char 336)
Location: query-blockstats at ../storage-daemon/qapi/../../qapi/block-core.json:1259

Error: Expecting property name enclosed in double quotes: line 7 column 19 (char 264)
Location: query-rocker-of-dpa-flows at ../qapi/rocker.json:256

Error: Expecting value: line 28 column 15 (char 775)
Location: query-spice at ../qapi/ui.json:372


If we have errors detected, we need to terminate the build by
making the generator exit, to force fixing of the problems.

This patch then needs to be moved to the end of the series, as
we need all the fixes applied to the schemas, before we enable
validation, to avoid breaking 'git bisect' build tests.


> +        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 316736b6a2..cf9beac3c9 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.41.0
> 
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v1 0/7] Validate and test qapi examples

[Daniel P. Berrangé]

On Tue, Sep 05, 2023 at 09:48:39PM +0200, Victor Toso wrote:
> Hi,
> 
> This is a follow up from the RFC sent in the end of 08-2022:
>     https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg04525.html
> 
> The generator code was rebased, without conflicts. The commit log was
> improved as per Markus suggestion [0], altough I'm sure it can be
> improved further.
> 
> To clarify, consuming the Examples as data for testing the qapi-go
> work has been very very helpful. I'm positive it can be of use for other
> bindings in the future, besides keeping the examples functional
> 
> Cheers,
> 
> [0] https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg04682.html
> 
> Victor Toso (7):
>   qapi: scripts: add a generator for qapi's examples
>   qapi: fix example of get-win32-socket command
>   qapi: fix example of dumpdtb command
>   qapi: fix example of cancel-vcpu-dirty-limit command
>   qapi: fix example of set-vcpu-dirty-limit command
>   qapi: fix example of calc-dirty-rate command
>   qapi: fix example of NETDEV_STREAM_CONNECTED event
> 
>  qapi/machine.json            |   2 +-
>  qapi/migration.json          |   6 +-
>  qapi/misc.json               |   2 +-
>  qapi/net.json                |   6 +-
>  scripts/qapi/dumpexamples.py | 194 +++++++++++++++++++++++++++++++++++
>  scripts/qapi/main.py         |   2 +
>  6 files changed, 204 insertions(+), 8 deletions(-)
>  create mode 100644 scripts/qapi/dumpexamples.py

After applying this series, aside from the extra broken examples
mentioned in my patch 1 comments, I also see a test suite failure
during build

FAILED: tests/qapi-builtin-types.c tests/qapi-builtin-types.h tests/qapi-builtin-visit.c tests/qapi-builtin-visit.h tests/test-qapi-commands-sub-sub-module.c tests/test-qapi-commands-sub-sub-module.h tests/test-qapi-commands.c tests/test-qapi-commands.h tests/test-qapi-emit-events.c tests/test-qapi-emit-events.h tests/test-qapi-events-sub-sub-module.c tests/test-qapi-events-sub-sub-module.h tests/test-qapi-events.c tests/test-qapi-events.h tests/test-qapi-init-commands.c tests/test-qapi-init-commands.h tests/test-qapi-introspect.c tests/test-qapi-introspect.h tests/test-qapi-types-sub-sub-module.c tests/test-qapi-types-sub-sub-module.h tests/test-qapi-types.c tests/test-qapi-types.h tests/test-qapi-visit-sub-sub-module.c tests/test-qapi-visit-sub-sub-module.h tests/test-qapi-visit.c tests/test-qapi-visit.h 
/home/berrange/src/virt/qemu/build/pyvenv/bin/python3 /home/berrange/src/virt/qemu/scripts/qapi-gen.py -o /home/berrange/src/virt/qemu/build/tests -b -p test- ../tests/qapi-schema/qapi-schema-test.json --suppress-tracing
Traceback (most recent call last):
  File "/home/berrange/src/virt/qemu/scripts/qapi-gen.py", line 19, in <module>
    sys.exit(main.main())
             ^^^^^^^^^^^
  File "/home/berrange/src/virt/qemu/scripts/qapi/main.py", line 96, in main
    generate(args.schema,
  File "/home/berrange/src/virt/qemu/scripts/qapi/main.py", line 58, in generate
    gen_examples(schema, output_dir, prefix)
  File "/home/berrange/src/virt/qemu/scripts/qapi/dumpexamples.py", line 40, in gen_examples
    schema.visit(vis)
  File "/home/berrange/src/virt/qemu/scripts/qapi/schema.py", line 1227, in visit
    mod.visit(visitor)
  File "/home/berrange/src/virt/qemu/scripts/qapi/schema.py", line 209, in visit
    entity.visit(visitor)
  File "/home/berrange/src/virt/qemu/scripts/qapi/schema.py", line 857, in visit
    visitor.visit_command(
  File "/home/berrange/src/virt/qemu/scripts/qapi/dumpexamples.py", line 184, in visit_command
    parse_examples_of(self, name)
  File "/home/berrange/src/virt/qemu/scripts/qapi/dumpexamples.py", line 118, in parse_examples_of
    assert((obj.doc is not None))
            ^^^^^^^^^^^^^^^^^^^
AssertionError
ninja: build stopped: subcommand failed.


not sure if that's related to the examples that still need fixing or not ?

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v1 0/7] Validate and test qapi examples

[Victor Toso]

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

Hi,

Thanks for the quick review Daniel!

On Wed, Sep 06, 2023 at 10:17:04AM +0100, Daniel P. Berrangé wrote:
> On Tue, Sep 05, 2023 at 09:48:39PM +0200, Victor Toso wrote:
> > Hi,
> > 
> > This is a follow up from the RFC sent in the end of 08-2022:
> >     https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg04525.html
> > 
> > The generator code was rebased, without conflicts. The commit log was
> > improved as per Markus suggestion [0], altough I'm sure it can be
> > improved further.
> > 
> > To clarify, consuming the Examples as data for testing the qapi-go
> > work has been very very helpful. I'm positive it can be of use for other
> > bindings in the future, besides keeping the examples functional
> > 
> > Cheers,
> > 
> > [0] https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg04682.html
> > 
> > Victor Toso (7):
> >   qapi: scripts: add a generator for qapi's examples
> >   qapi: fix example of get-win32-socket command
> >   qapi: fix example of dumpdtb command
> >   qapi: fix example of cancel-vcpu-dirty-limit command
> >   qapi: fix example of set-vcpu-dirty-limit command
> >   qapi: fix example of calc-dirty-rate command
> >   qapi: fix example of NETDEV_STREAM_CONNECTED event
> > 
> >  qapi/machine.json            |   2 +-
> >  qapi/migration.json          |   6 +-
> >  qapi/misc.json               |   2 +-
> >  qapi/net.json                |   6 +-
> >  scripts/qapi/dumpexamples.py | 194 +++++++++++++++++++++++++++++++++++
> >  scripts/qapi/main.py         |   2 +
> >  6 files changed, 204 insertions(+), 8 deletions(-)
> >  create mode 100644 scripts/qapi/dumpexamples.py
> 
> After applying this series, aside from the extra broken examples
> mentioned in my patch 1 comments, I also see a test suite failure
> during build

My bad.

> FAILED: tests/qapi-builtin-types.c tests/qapi-builtin-types.h tests/qapi-builtin-visit.c tests/qapi-builtin-visit.h tests/test-qapi-commands-sub-sub-module.c tests/test-qapi-commands-sub-sub-module.h tests/test-qapi-commands.c tests/test-qapi-commands.h tests/test-qapi-emit-events.c tests/test-qapi-emit-events.h tests/test-qapi-events-sub-sub-module.c tests/test-qapi-events-sub-sub-module.h tests/test-qapi-events.c tests/test-qapi-events.h tests/test-qapi-init-commands.c tests/test-qapi-init-commands.h tests/test-qapi-introspect.c tests/test-qapi-introspect.h tests/test-qapi-types-sub-sub-module.c tests/test-qapi-types-sub-sub-module.h tests/test-qapi-types.c tests/test-qapi-types.h tests/test-qapi-visit-sub-sub-module.c tests/test-qapi-visit-sub-sub-module.h tests/test-qapi-visit.c tests/test-qapi-visit.h 
> /home/berrange/src/virt/qemu/build/pyvenv/bin/python3 /home/berrange/src/virt/qemu/scripts/qapi-gen.py -o /home/berrange/src/virt/qemu/build/tests -b -p test- ../tests/qapi-schema/qapi-schema-test.json --suppress-tracing
> Traceback (most recent call last):
>   File "/home/berrange/src/virt/qemu/scripts/qapi-gen.py", line 19, in <module>
>     sys.exit(main.main())
>              ^^^^^^^^^^^
>   File "/home/berrange/src/virt/qemu/scripts/qapi/main.py", line 96, in main
>     generate(args.schema,
>   File "/home/berrange/src/virt/qemu/scripts/qapi/main.py", line 58, in generate
>     gen_examples(schema, output_dir, prefix)
>   File "/home/berrange/src/virt/qemu/scripts/qapi/dumpexamples.py", line 40, in gen_examples
>     schema.visit(vis)
>   File "/home/berrange/src/virt/qemu/scripts/qapi/schema.py", line 1227, in visit
>     mod.visit(visitor)
>   File "/home/berrange/src/virt/qemu/scripts/qapi/schema.py", line 209, in visit
>     entity.visit(visitor)
>   File "/home/berrange/src/virt/qemu/scripts/qapi/schema.py", line 857, in visit
>     visitor.visit_command(
>   File "/home/berrange/src/virt/qemu/scripts/qapi/dumpexamples.py", line 184, in visit_command
>     parse_examples_of(self, name)
>   File "/home/berrange/src/virt/qemu/scripts/qapi/dumpexamples.py", line 118, in parse_examples_of
>     assert((obj.doc is not None))
>             ^^^^^^^^^^^^^^^^^^^
> AssertionError
> ninja: build stopped: subcommand failed.
> 
> not sure if that's related to the examples that still need fixing or not ?

This is related to the script being fed with data without
documentation. In general, asserting should be the right approach
because we don't want API without docs but this failure comes
from the tests, that is, adding the following diff:

diff --git a/scripts/qapi/dumpexamples.py b/scripts/qapi/dumpexamples.py
index c14ed11774..a961c0575d 100644
--- a/scripts/qapi/dumpexamples.py
+++ b/scripts/qapi/dumpexamples.py
@@ -115,6 +115,10 @@ def parse_examples_of(self:
QAPISchemaGenExamplesVisitor,

     assert(name in self.schema._entity_dict)
     obj = self.schema._entity_dict[name]
+    if obj.doc is None:
+        print(f"{name} does not have documentation")
+        return
+
     assert((obj.doc is not None))
     module_name = obj._module.name

gives:

    user-def-cmd0 does not have documentation
    user-def-cmd does not have documentation
    user-def-cmd1 does not have documentation
    user-def-cmd2 does not have documentation
    cmd-success-response does not have documentation
    coroutine-cmd does not have documentation
    guest-get-time does not have documentation
    guest-sync does not have documentation
    boxed-struct does not have documentation
    boxed-union does not have documentation
    boxed-empty does not have documentation
    test-flags-command does not have documentation
    EVENT_A does not have documentation
    EVENT_B does not have documentation
    EVENT_C does not have documentation
    EVENT_D does not have documentation
    EVENT_E does not have documentation
    EVENT_F does not have documentation
    EVENT_G does not have documentation
    __ORG.QEMU_X-EVENT does not have documentation
    __org.qemu_x-command does not have documentation
    test-if-union-cmd does not have documentation
    test-if-alternate-cmd does not have documentation
    test-if-cmd does not have documentation
    test-cmd-return-def-three does not have documentation
    TEST_IF_EVENT does not have documentation
    TEST_IF_EVENT2 does not have documentation
    test-features0 does not have documentation
    test-command-features1 does not have documentation
    test-command-features3 does not have documentation
    test-command-cond-features1 does not have documentation
    test-command-cond-features2 does not have documentation
    test-command-cond-features3 does not have documentation
    TEST_EVENT_FEATURES0 does not have documentation
    TEST_EVENT_FEATURES1 does not have documentation
    TEST_EVENT_FEATURES2 does not have documentation

So, not sure if we should:
 1. Avoid asserting when running with tests
 2. Avoid running this generator with tests
 3. Add some minimal docs to the tests

Both (1) and (2) are quite simple. Not sure if there is real
benefit in (3). If we should tweak qemu tests with this, should
be related to using the JSON output itself, to keep examples
correct.

Cheers,
Victor

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

Re: [PATCH v1 1/7] qapi: scripts: add a generator for qapi's examples

[Victor Toso]

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

Hi,

On Wed, Sep 06, 2023 at 10:15:52AM +0100, Daniel P. Berrangé wrote:
> On Tue, Sep 05, 2023 at 09:48:40PM +0200, Victor Toso wrote:
> > This generator has two goals:
> >  1. Mechanical validation of QAPI examples
> >  2. Generate the examples in a JSON format to be consumed for extra
> >     validation.
> > 
> > The generator iterates over every Example section, parsing both server
> > and client messages. The generator prints any inconsistency found, for
> > example:
> > 
> >  |  Error: Extra data: line 1 column 39 (char 38)
> >  |  Location: cancel-vcpu-dirty-limit at qapi/migration.json:2017
> >  |  Data: {"execute": "cancel-vcpu-dirty-limit"},
> >  |      "arguments": { "cpu-index": 1 } }
> > 
> > The generator will output other 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, for example:
> > 
> >  | { "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": {} },
> >  |    } ]
> >  |    }
> >  |  ] }
> > 
> > Note that the order matters, as read by the Example section and
> > translated into "sequence-order". A language binding project can then
> > consume this files to Marshal and Unmarshal, comparing if the results
> > are what is to be expected.
> > 
> > RFC discussion:
> >     https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg04641.html
> > 
> > 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]))
> 
> This prints an error, but the caller ignores this and carries on
> as normal.
> 
> After applying this series, we still have multiple errors being
> printed on console

The first one is a easy to fix error. The other two are more
related to metadata inserted in valid examples, see:

> Error: Expecting ',' delimiter: line 12 column 19 (char 336)
> Location: query-blockstats at ../storage-daemon/qapi/../../qapi/block-core.json:1259

Indeed.
 
> Error: Expecting property name enclosed in double quotes: line 7 column 19 (char 264)
> Location: query-rocker-of-dpa-flows at ../qapi/rocker.json:256

    251 #                   "mask": {"in-pport": 4294901760}
    252 #                  },
 -> 253 #                  {...more...},
    254 #    ]}

> 
> Error: Expecting value: line 28 column 15 (char 775)
> Location: query-spice at ../qapi/ui.json:372

    365 #                "tls": false
    366 #             },
 -> 367 #             [ ... more channels follow ... ]
    368 #          ]

It would be good to have some sort of annotation for a valid
example, to express this is a long list and we are not putting
all of it here.

> If we have errors detected, we need to terminate the build by
> making the generator exit, to force fixing of the problems.

I agree. We just need to decided what to do with the above
mentioned extra metadata.

> This patch then needs to be moved to the end of the series, as
> we need all the fixes applied to the schemas, before we enable
> validation, to avoid breaking 'git bisect' build tests.

Makes sense. The rest of the series is not really dependent on
this one, so it can be applied if one wants it.

This patch needs rework at very least to not break the
test-suite.

Cheers,
Victor

> > +        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 316736b6a2..cf9beac3c9 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.41.0
> > 
> > 
> 
> With regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
> 

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

Re: [PATCH v1 0/7] Validate and test qapi examples

[Philippe Mathieu-Daudé]

On 7/9/23 20:17, Victor Toso wrote:
> Hi,

>>    File "/home/berrange/src/virt/qemu/scripts/qapi/dumpexamples.py", line 118, in parse_examples_of
>>      assert((obj.doc is not None))
>>              ^^^^^^^^^^^^^^^^^^^
>> AssertionError
>> ninja: build stopped: subcommand failed.
>>
>> not sure if that's related to the examples that still need fixing or not ?
> 
> This is related to the script being fed with data without
> documentation. In general, asserting should be the right approach
> because we don't want API without docs but this failure comes
> from the tests, that is, adding the following diff:
> 
> diff --git a/scripts/qapi/dumpexamples.py b/scripts/qapi/dumpexamples.py
> index c14ed11774..a961c0575d 100644
> --- a/scripts/qapi/dumpexamples.py
> +++ b/scripts/qapi/dumpexamples.py
> @@ -115,6 +115,10 @@ def parse_examples_of(self:
> QAPISchemaGenExamplesVisitor,
> 
>       assert(name in self.schema._entity_dict)
>       obj = self.schema._entity_dict[name]
> +    if obj.doc is None:
> +        print(f"{name} does not have documentation")
> +        return
> +
>       assert((obj.doc is not None))
>       module_name = obj._module.name
> 
> gives:
> 
>      user-def-cmd0 does not have documentation
>      user-def-cmd does not have documentation
[...]

> So, not sure if we should:
>   1. Avoid asserting when running with tests

This seems the most sensible option, adding an argument to
the 'command' invoked by meson's test_qapi_files() target in
tests/meson.build.

>   2. Avoid running this generator with tests
>   3. Add some minimal docs to the tests
> 
> Both (1) and (2) are quite simple. Not sure if there is real
> benefit in (3). If we should tweak qemu tests with this, should
> be related to using the JSON output itself, to keep examples
> correct.

IMO (3) is a waste of time.

Regards,

Phil.

> Cheers,
> Victor

Re: [PATCH v1 1/7] qapi: scripts: add a generator for qapi's examples

[Daniel P. Berrangé]

On Thu, Sep 07, 2023 at 08:34:07PM +0200, Victor Toso wrote:
> Hi,
> 
> On Wed, Sep 06, 2023 at 10:15:52AM +0100, Daniel P. Berrangé wrote:
> > On Tue, Sep 05, 2023 at 09:48:40PM +0200, Victor Toso wrote:
> > > This generator has two goals:
> > >  1. Mechanical validation of QAPI examples
> > >  2. Generate the examples in a JSON format to be consumed for extra
> > >     validation.
> > > 
> > > The generator iterates over every Example section, parsing both server
> > > and client messages. The generator prints any inconsistency found, for
> > > example:
> > > 
> > >  |  Error: Extra data: line 1 column 39 (char 38)
> > >  |  Location: cancel-vcpu-dirty-limit at qapi/migration.json:2017
> > >  |  Data: {"execute": "cancel-vcpu-dirty-limit"},
> > >  |      "arguments": { "cpu-index": 1 } }
> > > 
> > > The generator will output other 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, for example:
> > > 
> > >  | { "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": {} },
> > >  |    } ]
> > >  |    }
> > >  |  ] }
> > > 
> > > Note that the order matters, as read by the Example section and
> > > translated into "sequence-order". A language binding project can then
> > > consume this files to Marshal and Unmarshal, comparing if the results
> > > are what is to be expected.
> > > 
> > > RFC discussion:
> > >     https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg04641.html
> > > 
> > > 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]))
> > 
> > This prints an error, but the caller ignores this and carries on
> > as normal.
> > 
> > After applying this series, we still have multiple errors being
> > printed on console
> 
> The first one is a easy to fix error. The other two are more
> related to metadata inserted in valid examples, see:
> 
> > Error: Expecting ',' delimiter: line 12 column 19 (char 336)
> > Location: query-blockstats at ../storage-daemon/qapi/../../qapi/block-core.json:1259
> 
> Indeed.
>  
> > Error: Expecting property name enclosed in double quotes: line 7 column 19 (char 264)
> > Location: query-rocker-of-dpa-flows at ../qapi/rocker.json:256
> 
>     251 #                   "mask": {"in-pport": 4294901760}
>     252 #                  },
>  -> 253 #                  {...more...},
>     254 #    ]}
> 
> > 
> > Error: Expecting value: line 28 column 15 (char 775)
> > Location: query-spice at ../qapi/ui.json:372
> 
>     365 #                "tls": false
>     366 #             },
>  -> 367 #             [ ... more channels follow ... ]
>     368 #          ]
> 
> It would be good to have some sort of annotation for a valid
> example, to express this is a long list and we are not putting
> all of it here.

The second example already has 2 elements in the list, which
i think is sufficient illustration of "many" records.

The first example could just have a 2nd element added to its
returned list too I reckon

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v1 0/7] Validate and test qapi examples

[Daniel P. Berrangé]

On Fri, Sep 08, 2023 at 09:51:35AM +0200, Philippe Mathieu-Daudé wrote:
> On 7/9/23 20:17, Victor Toso wrote:
> > Hi,
> 
> > >    File "/home/berrange/src/virt/qemu/scripts/qapi/dumpexamples.py", line 118, in parse_examples_of
> > >      assert((obj.doc is not None))
> > >              ^^^^^^^^^^^^^^^^^^^
> > > AssertionError
> > > ninja: build stopped: subcommand failed.
> > > 
> > > not sure if that's related to the examples that still need fixing or not ?
> > 
> > This is related to the script being fed with data without
> > documentation. In general, asserting should be the right approach
> > because we don't want API without docs but this failure comes
> > from the tests, that is, adding the following diff:
> > 
> > diff --git a/scripts/qapi/dumpexamples.py b/scripts/qapi/dumpexamples.py
> > index c14ed11774..a961c0575d 100644
> > --- a/scripts/qapi/dumpexamples.py
> > +++ b/scripts/qapi/dumpexamples.py
> > @@ -115,6 +115,10 @@ def parse_examples_of(self:
> > QAPISchemaGenExamplesVisitor,
> > 
> >       assert(name in self.schema._entity_dict)
> >       obj = self.schema._entity_dict[name]
> > +    if obj.doc is None:
> > +        print(f"{name} does not have documentation")
> > +        return
> > +
> >       assert((obj.doc is not None))
> >       module_name = obj._module.name
> > 
> > gives:
> > 
> >      user-def-cmd0 does not have documentation
> >      user-def-cmd does not have documentation
> [...]
> 
> > So, not sure if we should:
> >   1. Avoid asserting when running with tests
> 
> This seems the most sensible option, adding an argument to
> the 'command' invoked by meson's test_qapi_files() target in
> tests/meson.build.
> 
> >   2. Avoid running this generator with tests
> >   3. Add some minimal docs to the tests
> > 
> > Both (1) and (2) are quite simple. Not sure if there is real
> > benefit in (3). If we should tweak qemu tests with this, should
> > be related to using the JSON output itself, to keep examples
> > correct.
> 
> IMO (3) is a waste of time.

Agreed, I think we shouuld just skip the docs check as this is not
loading a real QAPI schema, just a dummy test one.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v1 0/7] Validate and test qapi examples

[Victor Toso]

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

Hi,

On Fri, Sep 08, 2023 at 09:51:35AM +0200, Philippe Mathieu-Daudé wrote:
> On 7/9/23 20:17, Victor Toso wrote:
> > Hi,
> 
> > >    File "/home/berrange/src/virt/qemu/scripts/qapi/dumpexamples.py", line 118, in parse_examples_of
> > >      assert((obj.doc is not None))
> > >              ^^^^^^^^^^^^^^^^^^^
> > > AssertionError
> > > ninja: build stopped: subcommand failed.
> > > 
> > > not sure if that's related to the examples that still need fixing or not ?
> > 
> > This is related to the script being fed with data without
> > documentation. In general, asserting should be the right approach
> > because we don't want API without docs but this failure comes
> > from the tests, that is, adding the following diff:
> > 
> > diff --git a/scripts/qapi/dumpexamples.py b/scripts/qapi/dumpexamples.py
> > index c14ed11774..a961c0575d 100644
> > --- a/scripts/qapi/dumpexamples.py
> > +++ b/scripts/qapi/dumpexamples.py
> > @@ -115,6 +115,10 @@ def parse_examples_of(self:
> > QAPISchemaGenExamplesVisitor,
> > 
> >       assert(name in self.schema._entity_dict)
> >       obj = self.schema._entity_dict[name]
> > +    if obj.doc is None:
> > +        print(f"{name} does not have documentation")
> > +        return
> > +
> >       assert((obj.doc is not None))
> >       module_name = obj._module.name
> > 
> > gives:
> > 
> >      user-def-cmd0 does not have documentation
> >      user-def-cmd does not have documentation
> [...]
> 
> > So, not sure if we should:
> >   1. Avoid asserting when running with tests
> 
> This seems the most sensible option, adding an argument to
> the 'command' invoked by meson's test_qapi_files() target in
> tests/meson.build.

In a offline discussion with Markus, he pointed out to me the
existence of 'pragma': { 'doc-required': true }, which means we
can possibly avoid extra flags and changes in meson and handle
this in the generator. I'll update the code and submit, probably
Tomorrow :)

Cheers,
Victor

> >   2. Avoid running this generator with tests
> >   3. Add some minimal docs to the tests
> > 
> > Both (1) and (2) are quite simple. Not sure if there is real
> > benefit in (3). If we should tweak qemu tests with this, should
> > be related to using the JSON output itself, to keep examples
> > correct.
> 
> IMO (3) is a waste of time.
> 
> Regards,
> 
> Phil.
> 
> > Cheers,
> > Victor
> 

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