Re: [PATCH v2 4/4] qapi: expose all schema features to code

["Daniel P. Berrangé" <berrange@redhat.com>]

On Fri, Nov 15, 2024 at 08:47:20AM +0100, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Thu, Nov 14, 2024 at 01:48:28PM +0100, Markus Armbruster wrote:
> >> Daniel P. Berrangé <berrange@redhat.com> writes:
> >> 
> >> > This replaces use of the constants from the QapiSpecialFeatures
> >> > enum, with constants from the auto-generate QapiFeatures enum
> >> > in qapi-features.h
> >> >
> >> > The 'deprecated' and 'unstable' features still have a little bit of
> >> > special handling, being force defined to be the 1st + 2nd features
> >> > in the enum, regardless of whether they're used in the schema. This
> >> > retains compatibility with common code that references the features
> >> > via the QapiSpecialFeatures constants.
> >> >
> >> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> >> > ---
> >> >  meson.build              |  1 +
> >> >  scripts/qapi/commands.py |  1 +
> >> >  scripts/qapi/features.py | 62 ++++++++++++++++++++++++++++++++++++++++
> >> >  scripts/qapi/gen.py      |  4 +--
> >> >  scripts/qapi/main.py     |  2 ++
> >> >  scripts/qapi/schema.py   | 19 +++++++++++-
> >> >  scripts/qapi/types.py    |  6 ++--
> >> >  scripts/qapi/visit.py    |  3 +-
> >> >  8 files changed, 92 insertions(+), 6 deletions(-)
> >> >  create mode 100644 scripts/qapi/features.py

> >> I further guess you sort the non-special features just to make the
> >> generated code easier for humans to navigate.
> >> 
> >> Correct?
> >
> > The remaining sort was just to give a predictable stable output,
> > should QAPI usage of features be reordered.
> 
> We don't do that for enum QAPIEvent, and it hasn't inconvenienced us as
> far as I can tell.  No big deal, I just like consistency.

Sure, I'll removethe sorting

> >> pycodestyle gripes
> >> 
> >>     scripts/qapi/features.py:57:1: E302 expected 2 blank lines, found 1
> >> 
> >> This part generates a C enum.  It's similar to gen_enum() from types.py,
> >> except we work with a list of QAPISchemaFeature here, and a list of
> >> QAPISchemaEnumMember there.
> >> 
> >> To reuse gen_enum() here, we'd have to make up a member list, like we do
> >> in events.py for enum QAPIEvent.
> >
> > I'll have a look at that.
> 
> Reuse it only if it's easy for you.  We can always improve on top.

Using gen_enum will create the   enum <-> string conversion table,
and I'm not sure we need/want that for the special features ?


> >> We commonly use None as info value for built-in stuff, and that's why
> >> it's Optional[QAPISourceInfo], not just QAPISourceInfo.
> >
> > Yeah, not sure what I was thinking here, looking again I
> > should have passed "None"
> >
> >> But do we really need to make up some QAPISchemaFeature?  Hmm.  The
> >> appended patch dumbs down ._feature_dict to a set.
> 
> See below for a possible reason to keep .feature_dict.
> 
> > I was following the same pattern as self._entity_dict and
> > self._module_dict, rather than dumbing down to the bare
> > minimum needed by my current use case. I don't mind which
> > strategy we take.
> 
> .entity_dict maps the name to the one entity.  Likewise .module_dict.
> .feature_dict, however, maps it to the first of possibly many.  That's
> not wrong, just peculiar and possibly less than obvious to readers who
> aren't familiar with how we represent features internally.  Worth a
> comment?

I'll comment it.


> >> However, the reporting is less than nice:
> >> 
> >>     $ python scripts/qapi-gen.py -o $$ tests/qapi-schema/features-too-many.json 
> >>     Traceback (most recent call last):
> >>       File "/work/armbru/qemu/scripts/qapi-gen.py", line 19, in <module>
> >>         sys.exit(main.main())
> >>                  ^^^^^^^^^^^
> >>       File "/work/armbru/qemu/scripts/qapi/main.py", line 96, in main
> >>         generate(args.schema,
> >>       File "/work/armbru/qemu/scripts/qapi/main.py", line 51, in generate
> >>         schema = QAPISchema(schema_file)
> >>                  ^^^^^^^^^^^^^^^^^^^^^^^
> >>       File "/work/armbru/qemu/scripts/qapi/schema.py", line 1155, in __init__
> >>         self._def_exprs(exprs)
> >>       File "/work/armbru/qemu/scripts/qapi/schema.py", line 1482, in _def_exprs
> >>         self._def_struct_type(expr)
> >>       File "/work/armbru/qemu/scripts/qapi/schema.py", line 1377, in _def_struct_type
> >>         features = self._make_features(expr.get('features'), info)
> >>                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >>       File "/work/armbru/qemu/scripts/qapi/schema.py", line 1274, in _make_features
> >>         raise Exception("Maximum of 64 schema features is permitted")
> >>     Exception: Maximum of 64 schema features is permitted
> >
> > Is there any better way to approach this error reporting ?
> 
> Raise QAPISemError in .check().
> 
> Hmm, then you need a QAPISourceInfo to pass.  .feature_dict will give
> you one: the .info of the feature added last.

This also points out that I failed to add a test case for this
"too many features' scenario, which I'll fix too.

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