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 :|