On Fri, Nov 15, 2024 at 08:47:20AM +0100, Markus Armbruster wrote:
> Daniel P. Berrangé <[email protected]> writes:
>
> > On Thu, Nov 14, 2024 at 01:48:28PM +0100, Markus Armbruster wrote:
> >> Daniel P. Berrangé <[email protected]> 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é <[email protected]>
> >> > ---
> >> > 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 :|
