If the schema itself is extended in qemu we need to have a notification
to add appropriate handling to ensure that we have full coverage of all
fields.
Add validation that only fields that libvirt currently knows about are
present in the schema.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
scripts/qemu-replies-tool.py | 145 +++++++++++++++++++++++++++++++++++
1 file changed, 145 insertions(+)
diff --git a/scripts/qemu-replies-tool.py b/scripts/qemu-replies-tool.py
index 57bb26a356..c5dee9a66a 100755
--- a/scripts/qemu-replies-tool.py
+++ b/scripts/qemu-replies-tool.py
@@ -15,6 +15,10 @@ class qrtException(Exception):
pass
+class qmpSchemaException(Exception):
+ pass
+
+
# Load the 'replies' file into a list of (command, reply) tuples of parsed JSON
def qemu_replies_load(filename):
conv = []
@@ -153,17 +157,157 @@ def modify_replies(conv):
conv.insert(idx, (cmd, reply_unsupp))
+# Validates that 'entry' (an member of the QMP schema):
+# - checks that it's a Dict (imported from a JSON object)
+# - checks that all 'mandatory' fields are present and their types match
+# - checks the types of all 'optional' fields
+# - checks that no unknown fields are present
+def validate_qmp_schema_check_keys(entry, mandatory, optional):
+ keys = set(entry.keys())
+
+ for k, t in mandatory:
+ try:
+ keys.remove(k)
+ except KeyError:
+ raise qmpSchemaException("missing mandatory key '%s' in schema '%s'" % (k, entry))
+
+ if not isinstance(entry[k], t):
+ raise qmpSchemaException("key '%s' is not of the expected type '%s' in schema '%s'" % (k, t, entry))
+
+ for k, t in optional:
+ if k in keys:
+ keys.discard(k)
+
+ if t is not None:
+ if not isinstance(entry[k], t):
+ raise qmpSchemaException("key '%s' is not of the expected type '%s' in schema '%s'" % (k, t, entry))
+
+ if len(keys) > 0:
+ raise qmpSchemaException("unhandled keys '%s' in schema '%s'" % (','.join(list(keys)), entry))
+
+
+# Validates the optional 'features' and that they consist only of strings
+def validate_qmp_schema_check_features_list(entry):
+ for f in entry.get('features', []):
+ if not isinstance(f, str):
+ raise qmpSchemaException("broken 'features' list in schema entry '%s'" % entry)
+
+
+# Validate that the passed schema has only members supported by this script and
+# by the libvirt internals. This is useful to stay up to date with any changes
+# to the schema.
+def validate_qmp_schema(schemalist):
+ for entry in schemalist:
+ if not isinstance(entry, dict):
+ raise qmpSchemaException("schema entry '%s' is not a JSON Object (dict)" % (entry))
+
+ match entry.get('meta-type', None):
+ case 'command':
+ validate_qmp_schema_check_keys(entry,
+ mandatory=[('name', str),
+ ('meta-type', str),
+ ('arg-type', str),
+ ('ret-type', str)],
+ optional=[('features', list),
+ ('allow-oob', bool)])
+
+ validate_qmp_schema_check_features_list(entry)
+
+ case 'event':
+ validate_qmp_schema_check_keys(entry,
+ mandatory=[('name', str),
+ ('meta-type', str),
+ ('arg-type', str)],
+ optional=[('features', list)])
+
+ validate_qmp_schema_check_features_list(entry)
+
+ case 'object':
+ validate_qmp_schema_check_keys(entry,
+ mandatory=[('name', str),
+ ('meta-type', str),
+ ('members', list)],
+ optional=[('tag', str),
+ ('variants', list),
+ ('features', list)])
+
+ validate_qmp_schema_check_features_list(entry)
+
+ for m in entry.get('members', []):
+ validate_qmp_schema_check_keys(m,
+ mandatory=[('name', str),
+ ('type', str)],
+ optional=[('default', None),
+ ('features', list)])
+ validate_qmp_schema_check_features_list(m)
+
+ for m in entry.get('variants', []):
+ validate_qmp_schema_check_keys(m,
+ mandatory=[('case', str),
+ ('type', str)],
+ optional=[])
+
+ case 'array':
+ validate_qmp_schema_check_keys(entry,
+ mandatory=[('name', str),
+ ('meta-type', str),
+ ('element-type', str)],
+ optional=[])
+
+ case 'enum':
+ validate_qmp_schema_check_keys(entry,
+ mandatory=[('name', str),
+ ('meta-type', str)],
+ optional=[('members', list),
+ ('values', list)])
+
+ for m in entry.get('members', []):
+ validate_qmp_schema_check_keys(m,
+ mandatory=[('name', str)],
+ optional=[('features', list)])
+ validate_qmp_schema_check_features_list(m)
+
+ case 'alternate':
+ validate_qmp_schema_check_keys(entry,
+ mandatory=[('name', str),
+ ('meta-type', str),
+ ('members', list)],
+ optional=[])
+
+ for m in entry.get('members', []):
+ validate_qmp_schema_check_keys(m,
+ mandatory=[('type', str)],
+ optional=[])
+
+ case 'builtin':
+ validate_qmp_schema_check_keys(entry,
+ mandatory=[('name', str),
+ ('meta-type', str),
+ ('json-type', str)],
+ optional=[])
+
+ case _:
+ raise qmpSchemaException("unknown or missing 'meta-type' in schema entry '%s'" % entry)
+
+
def process_one(filename, args):
try:
conv = qemu_replies_load(filename)
modify_replies(conv)
+ for (cmd, rep) in conv:
+ if cmd['execute'] == 'query-qmp-schema':
+ validate_qmp_schema(rep['return'])
+
qemu_replies_compare_or_replace(filename, conv, args.regenerate)
except qrtException as e:
print("'%s' ... FAIL\n%s" % (filename, e))
return False
+ except qmpSchemaException as qe:
+ print("'%s' ... FAIL\nqmp schema error: %s" % (filename, qe))
+ return False
print("'%s' ... OK" % filename)
return True
@@ -181,6 +325,7 @@ The default mode is validation which checks the following:
- each command has a reply and both are valid JSON
- numbering of the 'id' field is as expected
- the input file has the expected JSON formatting
+ - the QMP schema from qemu is fully covered by libvirt's code
The tool can be also used to programmaticaly modify the '.replies' file by
editting the 'modify_replies' method directly in the source, or for
--
2.43.0
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
On Tue, Jan 16, 2024 at 17:12:41 +0100, Peter Krempa wrote:
> If the schema itself is extended in qemu we need to have a notification
> to add appropriate handling to ensure that we have full coverage of all
> fields.
>
> Add validation that only fields that libvirt currently knows about are
> present in the schema.
>
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
> scripts/qemu-replies-tool.py | 145 +++++++++++++++++++++++++++++++++++
> 1 file changed, 145 insertions(+)
[...]
> +# Validate that the passed schema has only members supported by this script and
> +# by the libvirt internals. This is useful to stay up to date with any changes
> +# to the schema.
> +def validate_qmp_schema(schemalist):
> + for entry in schemalist:
> + if not isinstance(entry, dict):
> + raise qmpSchemaException("schema entry '%s' is not a JSON Object (dict)" % (entry))
> +
> + match entry.get('meta-type', None):
> + case 'command':
Gah, it turns out that match/case was introduced in python 3.10 and
based on CI failure we need to target older versions, thus I'll need to
convert this to if/elif or something equivalent.
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
On Tue, Jan 16, 2024 at 05:12:41PM +0100, Peter Krempa wrote:
> +def validate_qmp_schema_check_keys(entry, mandatory, optional):
> + keys = set(entry.keys())
> +
> + for k, t in mandatory:
> + try:
> + keys.remove(k)
> + except KeyError:
> + raise qmpSchemaException("missing mandatory key '%s' in schema '%s'" % (k, entry))
> +
> + if not isinstance(entry[k], t):
> + raise qmpSchemaException("key '%s' is not of the expected type '%s' in schema '%s'" % (k, t, entry))
> +
> + for k, t in optional:
> + if k in keys:
> + keys.discard(k)
> +
> + if t is not None:
> + if not isinstance(entry[k], t):
> + raise qmpSchemaException("key '%s' is not of the expected type '%s' in schema '%s'" % (k, t, entry))
This doesn't cover the case where the value for the key is supposed
to be JSON null. You can either change this to something like
if ((t is not None and not isinstance(entry[k], t)) or
(t is None and entry[k] is not None)):
raise qmpSchemaException(...)
or, more simply, drop the check on t being None...
> +def validate_qmp_schema(schemalist):
> + for entry in schemalist:
> + if not isinstance(entry, dict):
> + raise qmpSchemaException("schema entry '%s' is not a JSON Object (dict)" % (entry))
> +
> + match entry.get('meta-type', None):
> + case 'object':
> + for m in entry.get('members', []):
> + validate_qmp_schema_check_keys(m,
> + mandatory=[('name', str),
> + ('type', str)],
> + optional=[('default', None),
... and change the second member of the tuple to type(None) here.
> def process_one(filename, args):
> try:
> conv = qemu_replies_load(filename)
>
> modify_replies(conv)
>
> + for (cmd, rep) in conv:
> + if cmd['execute'] == 'query-qmp-schema':
> + validate_qmp_schema(rep['return'])
Should we check whether query-qmp-schema and its output are in fact
present in the replies file? Or can we just assume that they're going
to be there?
--
Andrea Bolognani / Red Hat / Virtualization
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
On Thu, Jan 25, 2024 at 09:47:11 -0800, Andrea Bolognani wrote:
> On Tue, Jan 16, 2024 at 05:12:41PM +0100, Peter Krempa wrote:
> > +def validate_qmp_schema_check_keys(entry, mandatory, optional):
> > + keys = set(entry.keys())
> > +
> > + for k, t in mandatory:
> > + try:
> > + keys.remove(k)
> > + except KeyError:
> > + raise qmpSchemaException("missing mandatory key '%s' in schema '%s'" % (k, entry))
> > +
> > + if not isinstance(entry[k], t):
> > + raise qmpSchemaException("key '%s' is not of the expected type '%s' in schema '%s'" % (k, t, entry))
> > +
> > + for k, t in optional:
> > + if k in keys:
> > + keys.discard(k)
> > +
> > + if t is not None:
> > + if not isinstance(entry[k], t):
> > + raise qmpSchemaException("key '%s' is not of the expected type '%s' in schema '%s'" % (k, t, entry))
>
> This doesn't cover the case where the value for the key is supposed
> to be JSON null. You can either change this to something like
>
> if ((t is not None and not isinstance(entry[k], t)) or
> (t is None and entry[k] is not None)):
> raise qmpSchemaException(...)
>
> or, more simply, drop the check on t being None...
>
> > +def validate_qmp_schema(schemalist):
> > + for entry in schemalist:
> > + if not isinstance(entry, dict):
> > + raise qmpSchemaException("schema entry '%s' is not a JSON Object (dict)" % (entry))
> > +
> > + match entry.get('meta-type', None):
> > + case 'object':
> > + for m in entry.get('members', []):
> > + validate_qmp_schema_check_keys(m,
> > + mandatory=[('name', str),
> > + ('type', str)],
> > + optional=[('default', None),
>
> ... and change the second member of the tuple to type(None) here.
>
> > def process_one(filename, args):
> > try:
> > conv = qemu_replies_load(filename)
> >
> > modify_replies(conv)
> >
> > + for (cmd, rep) in conv:
> > + if cmd['execute'] == 'query-qmp-schema':
> > + validate_qmp_schema(rep['return'])
>
> Should we check whether query-qmp-schema and its output are in fact
> present in the replies file? Or can we just assume that they're going
> to be there?
It's not needed here. The qemu capability probing code (which is
excercised via 'qemucapabilitiestest') calls it unconditionally.
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
On Fri, Jan 26, 2024 at 09:33:13AM +0100, Peter Krempa wrote:
> On Thu, Jan 25, 2024 at 09:47:11 -0800, Andrea Bolognani wrote:
> > On Tue, Jan 16, 2024 at 05:12:41PM +0100, Peter Krempa wrote:
> > > +def validate_qmp_schema_check_keys(entry, mandatory, optional):
> > > + keys = set(entry.keys())
> > > +
> > > + for k, t in mandatory:
> > > + try:
> > > + keys.remove(k)
> > > + except KeyError:
> > > + raise qmpSchemaException("missing mandatory key '%s' in schema '%s'" % (k, entry))
> > > +
> > > + if not isinstance(entry[k], t):
> > > + raise qmpSchemaException("key '%s' is not of the expected type '%s' in schema '%s'" % (k, t, entry))
> > > +
> > > + for k, t in optional:
> > > + if k in keys:
> > > + keys.discard(k)
> > > +
> > > + if t is not None:
> > > + if not isinstance(entry[k], t):
> > > + raise qmpSchemaException("key '%s' is not of the expected type '%s' in schema '%s'" % (k, t, entry))
> >
> > This doesn't cover the case where the value for the key is supposed
> > to be JSON null. You can either change this to something like
> >
> > if ((t is not None and not isinstance(entry[k], t)) or
> > (t is None and entry[k] is not None)):
> > raise qmpSchemaException(...)
> >
> > or, more simply, drop the check on t being None...
> >
> > > +def validate_qmp_schema(schemalist):
> > > + for entry in schemalist:
> > > + if not isinstance(entry, dict):
> > > + raise qmpSchemaException("schema entry '%s' is not a JSON Object (dict)" % (entry))
> > > +
> > > + match entry.get('meta-type', None):
> > > + case 'object':
> > > + for m in entry.get('members', []):
> > > + validate_qmp_schema_check_keys(m,
> > > + mandatory=[('name', str),
> > > + ('type', str)],
> > > + optional=[('default', None),
> >
> > ... and change the second member of the tuple to type(None) here.
> >
> > > def process_one(filename, args):
> > > try:
> > > conv = qemu_replies_load(filename)
> > >
> > > modify_replies(conv)
> > >
> > > + for (cmd, rep) in conv:
> > > + if cmd['execute'] == 'query-qmp-schema':
> > > + validate_qmp_schema(rep['return'])
> >
> > Should we check whether query-qmp-schema and its output are in fact
> > present in the replies file? Or can we just assume that they're going
> > to be there?
>
> It's not needed here. The qemu capability probing code (which is
> excercised via 'qemucapabilitiestest') calls it unconditionally.
Sounds good.
Please consider acting on the other piece of feedback above before
pushing, but either way
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
--
Andrea Bolognani / Red Hat / Virtualization
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
On Fri, Jan 26, 2024 at 06:40:23 -0800, Andrea Bolognani wrote:
> On Fri, Jan 26, 2024 at 09:33:13AM +0100, Peter Krempa wrote:
> > On Thu, Jan 25, 2024 at 09:47:11 -0800, Andrea Bolognani wrote:
> > > On Tue, Jan 16, 2024 at 05:12:41PM +0100, Peter Krempa wrote:
> > > > +def validate_qmp_schema_check_keys(entry, mandatory, optional):
[...]
> > > > + for k, t in optional:
> > > > + if k in keys:
> > > > + keys.discard(k)
> > > > +
> > > > + if t is not None:
> > > > + if not isinstance(entry[k], t):
> > > > + raise qmpSchemaException("key '%s' is not of the expected type '%s' in schema '%s'" % (k, t, entry))
> > >
> > > This doesn't cover the case where the value for the key is supposed
> > > to be JSON null. You can either change this to something like
> > >
> > > if ((t is not None and not isinstance(entry[k], t)) or
> > > (t is None and entry[k] is not None)):
> > > raise qmpSchemaException(...)
> > >
> > > or, more simply, drop the check on t being None...
> > >
> > > > +def validate_qmp_schema(schemalist):
> > > > + for entry in schemalist:
> > > > + if not isinstance(entry, dict):
> > > > + raise qmpSchemaException("schema entry '%s' is not a JSON Object (dict)" % (entry))
> > > > +
> > > > + match entry.get('meta-type', None):
> > > > + case 'object':
> > > > + for m in entry.get('members', []):
> > > > + validate_qmp_schema_check_keys(m,
> > > > + mandatory=[('name', str),
> > > > + ('type', str)],
> > > > + optional=[('default', None),
> > >
> > > ... and change the second member of the tuple to type(None) here.a
I liked this version in the end.
> > >
> > > > def process_one(filename, args):
> > > > try:
> > > > conv = qemu_replies_load(filename)
> > > >
> > > > modify_replies(conv)
> > > >
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
© 2016 - 2025 Red Hat, Inc.