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(a)redhat.com>
--
Andrea Bolognani / Red Hat / Virtualization