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 <[email protected]>
--
Andrea Bolognani / Red Hat / Virtualization
_______________________________________________
Devel mailing list -- [email protected]
To unsubscribe send an email to [email protected]
