The 'command-features' pragma allows for defining additional
special features that are unique to a particular QAPI schema
instance and its implementation.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
scripts/qapi/parser.py | 2 ++
scripts/qapi/source.py | 2 ++
2 files changed, 4 insertions(+)
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 7b13a583ac..36a9046243 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -243,6 +243,8 @@ def check_list_str(name: str, value: object) -> List[str]:
pragma.documentation_exceptions = check_list_str(name, value)
elif name == 'member-name-exceptions':
pragma.member_name_exceptions = check_list_str(name, value)
+ elif name == 'command-features':
+ pragma.command_features = check_list_str(name, value)
else:
raise QAPISemError(info, "unknown pragma '%s'" % name)
diff --git a/scripts/qapi/source.py b/scripts/qapi/source.py
index 7b379fdc92..07c2958ac4 100644
--- a/scripts/qapi/source.py
+++ b/scripts/qapi/source.py
@@ -28,6 +28,8 @@ def __init__(self) -> None:
self.documentation_exceptions: List[str] = []
# Types whose member names may violate case conventions
self.member_name_exceptions: List[str] = []
+ # Arbitrary extra features recorded against commands
+ self.command_features: List[str] = []
class QAPISourceInfo:
--
2.45.1
Daniel P. Berrangé <berrange@redhat.com> writes: > The 'command-features' pragma allows for defining additional > special features that are unique to a particular QAPI schema > instance and its implementation. So far, we have special features (predefined, known to the generator and treated specially), and normal features (user-defined, not known to the generator). You create a new kind in between: user-defined, not known to the generator, yet treated specially (I guess?). Hmm. Could you at least hint at indented use here? What special treatment do you have in mind? > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > --- > scripts/qapi/parser.py | 2 ++ > scripts/qapi/source.py | 2 ++ > 2 files changed, 4 insertions(+) > > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py > index 7b13a583ac..36a9046243 100644 > --- a/scripts/qapi/parser.py > +++ b/scripts/qapi/parser.py > @@ -243,6 +243,8 @@ def check_list_str(name: str, value: object) -> List[str]: > pragma.documentation_exceptions = check_list_str(name, value) > elif name == 'member-name-exceptions': > pragma.member_name_exceptions = check_list_str(name, value) > + elif name == 'command-features': > + pragma.command_features = check_list_str(name, value) > else: > raise QAPISemError(info, "unknown pragma '%s'" % name) > > diff --git a/scripts/qapi/source.py b/scripts/qapi/source.py > index 7b379fdc92..07c2958ac4 100644 > --- a/scripts/qapi/source.py > +++ b/scripts/qapi/source.py > @@ -28,6 +28,8 @@ def __init__(self) -> None: > self.documentation_exceptions: List[str] = [] > # Types whose member names may violate case conventions > self.member_name_exceptions: List[str] = [] > + # Arbitrary extra features recorded against commands > + self.command_features: List[str] = [] > > > class QAPISourceInfo:
On Fri, Jul 12, 2024 at 10:07:34AM +0200, Markus Armbruster wrote: > Daniel P. Berrangé <berrange@redhat.com> writes: > > > The 'command-features' pragma allows for defining additional > > special features that are unique to a particular QAPI schema > > instance and its implementation. > > So far, we have special features (predefined, known to the generator and > treated specially), and normal features (user-defined, not known to the > generator). You create a new kind in between: user-defined, not known > to the generator, yet treated specially (I guess?). Hmm. > > Could you at least hint at indented use here? What special treatment do > you have in mind? Essentially, these features are a way to attach metadata to commands that the server side impl can later query. This eliminates the need to hardcode lists of commands, such as in QGA which hardcodes a list of commands which are safe to use when filesystems are frozen. This is illustrated later in this series. > > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > > --- > > scripts/qapi/parser.py | 2 ++ > > scripts/qapi/source.py | 2 ++ > > 2 files changed, 4 insertions(+) > > > > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py > > index 7b13a583ac..36a9046243 100644 > > --- a/scripts/qapi/parser.py > > +++ b/scripts/qapi/parser.py > > @@ -243,6 +243,8 @@ def check_list_str(name: str, value: object) -> List[str]: > > pragma.documentation_exceptions = check_list_str(name, value) > > elif name == 'member-name-exceptions': > > pragma.member_name_exceptions = check_list_str(name, value) > > + elif name == 'command-features': > > + pragma.command_features = check_list_str(name, value) > > else: > > raise QAPISemError(info, "unknown pragma '%s'" % name) > > > > diff --git a/scripts/qapi/source.py b/scripts/qapi/source.py > > index 7b379fdc92..07c2958ac4 100644 > > --- a/scripts/qapi/source.py > > +++ b/scripts/qapi/source.py > > @@ -28,6 +28,8 @@ def __init__(self) -> None: > > self.documentation_exceptions: List[str] = [] > > # Types whose member names may violate case conventions > > self.member_name_exceptions: List[str] = [] > > + # Arbitrary extra features recorded against commands > > + self.command_features: List[str] = [] > > > > > > class QAPISourceInfo: > 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 :|
Daniel P. Berrangé <berrange@redhat.com> writes: > On Fri, Jul 12, 2024 at 10:07:34AM +0200, Markus Armbruster wrote: >> Daniel P. Berrangé <berrange@redhat.com> writes: >> >> > The 'command-features' pragma allows for defining additional >> > special features that are unique to a particular QAPI schema >> > instance and its implementation. >> >> So far, we have special features (predefined, known to the generator and >> treated specially), and normal features (user-defined, not known to the >> generator). You create a new kind in between: user-defined, not known >> to the generator, yet treated specially (I guess?). Hmm. >> >> Could you at least hint at indented use here? What special treatment do >> you have in mind? > > Essentially, these features are a way to attach metadata to commands that > the server side impl can later query. This eliminates the need to hardcode > lists of commands, such as in QGA which hardcodes a list of commands which > are safe to use when filesystems are frozen. This is illustrated later in > this series. Please update docs/devel/qapi-code-gen.rst section "Pragma directives", and maybe section "Features". I'm not sure conflating the new kind of feature with existing special features is a good idea. I need to review more of the series before I can make up my mind.
On Fri, Jul 12, 2024 at 10:50:54AM +0200, Markus Armbruster wrote: > Daniel P. Berrangé <berrange@redhat.com> writes: > > > On Fri, Jul 12, 2024 at 10:07:34AM +0200, Markus Armbruster wrote: > >> Daniel P. Berrangé <berrange@redhat.com> writes: > >> > >> > The 'command-features' pragma allows for defining additional > >> > special features that are unique to a particular QAPI schema > >> > instance and its implementation. > >> > >> So far, we have special features (predefined, known to the generator and > >> treated specially), and normal features (user-defined, not known to the > >> generator). You create a new kind in between: user-defined, not known > >> to the generator, yet treated specially (I guess?). Hmm. > >> > >> Could you at least hint at indented use here? What special treatment do > >> you have in mind? > > > > Essentially, these features are a way to attach metadata to commands that > > the server side impl can later query. This eliminates the need to hardcode > > lists of commands, such as in QGA which hardcodes a list of commands which > > are safe to use when filesystems are frozen. This is illustrated later in > > this series. > > Please update docs/devel/qapi-code-gen.rst section "Pragma directives", > and maybe section "Features". > > I'm not sure conflating the new kind of feature with existing special > features is a good idea. I need to review more of the series before I > can make up my mind. I originally implemented a completely separate 'tags' concept in the QAPI parser, before deciding I was just re-inventing 'features' for no obvious benefit. The other nice thing about using features is that these are exposed in the schema and docs. With the 'fsfreeze' restriction in code, there's no formal docs of what commands are allowed when frozen, and this is also not exposed in QAPI schema to apps. Using 'features' we get all that as standard. 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 :|
Sorry for the delay; too many distractions, and I needed a good think.
Daniel P. Berrangé <berrange@redhat.com> writes:
> On Fri, Jul 12, 2024 at 10:50:54AM +0200, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>>
>> > On Fri, Jul 12, 2024 at 10:07:34AM +0200, Markus Armbruster wrote:
>> >> Daniel P. Berrangé <berrange@redhat.com> writes:
>> >>
>> >> > The 'command-features' pragma allows for defining additional
>> >> > special features that are unique to a particular QAPI schema
>> >> > instance and its implementation.
>> >>
>> >> So far, we have special features (predefined, known to the generator and
>> >> treated specially), and normal features (user-defined, not known to the
>> >> generator). You create a new kind in between: user-defined, not known
>> >> to the generator, yet treated specially (I guess?). Hmm.
>> >>
>> >> Could you at least hint at indented use here? What special treatment do
>> >> you have in mind?
>> >
>> > Essentially, these features are a way to attach metadata to commands that
>> > the server side impl can later query. This eliminates the need to hardcode
>> > lists of commands, such as in QGA which hardcodes a list of commands which
>> > are safe to use when filesystems are frozen. This is illustrated later in
>> > this series.
>>
>> Please update docs/devel/qapi-code-gen.rst section "Pragma directives",
>> and maybe section "Features".
Second thoughts; see below.
>> I'm not sure conflating the new kind of feature with existing special
>> features is a good idea. I need to review more of the series before I
>> can make up my mind.
>
> I originally implemented a completely separate 'tags' concept in the
> QAPI parser, before deciding I was just re-inventing 'features' for
> no obvious benefit.
>
> The other nice thing about using features is that these are exposed
> in the schema and docs. With the 'fsfreeze' restriction in code,
> there's no formal docs of what commands are allowed when frozen, and
> this is also not exposed in QAPI schema to apps. Using 'features'
> we get all that as standard.
When you need to tack a mark to one or more things for whatever purpose
*and* expose it to QMP clients, then features make sense. This is the
case here.
Initially, features were strictly an external interface annotation, and
were not meant to be used within QEMU. All features were user-defined.
This changed when I created configurable policy for deprecated and
unstable management interfaces: the policy engine needs to check for
features 'deprecated' and 'unstable'. Since the policy engine is partly
implemented in generated code, these two features need to be baked into
the generator. This makes them special.
You need less than that: a predicate "does <command> have <feature>" for
certain features, ideally without baking them into the generator.
The command registry already tracks each command's special features for
use by the policy engine. Obvious idea: also track the features you
want to pass to the predicate.
Your series adds tracking for exactly the features you need:
* Enumerate them in the schema with new pragma command-features
Missing: documentation for the pragma.
* Generate an extension QapiSpecialFeatureCustom of existing enum
QapiSpecialFeature, which is not generated. The latter is in
qapi/util.h, the former in ${prefix}qapi-init-commands.h.
* Mark these features special for commands only, so existing registry
machinery tracks them. Do *not* make them special elsewhere, because
that would break things.
Feels like a hack. Minor trap: if you use the same feature in
multiple schemas, multiple generated headers will define the same enum
constant, possibly with different values. If you manage to include
the wrong header *and* the value differs there, you'll likely lose
hair.
* Missing: tests.
I think we can avoid supplying most of the missing bits. The main QAPI
schema uses five features: deprecated, unstable,
allow-write-only-overlay, dynamic-auto-read-only, fdset. The QGA QAPI
schema uses four, namely the four you add in this series. Why not track
all features, and dispense with the pragma? Like this:
* Change type of feature bitsets to uint64_t (it's unsigned now).
* Error out if a schema has more than 64 features.
* Move enum QapiSpecialFeature into a new generated header.
* Generate a member for each feature, not just the two predefined ones.
* Pass all command features to the registry, not just the special ones.
* Recommended: do the same elsewhere, i.e. replace
gen_special_features() by gen_features().
Thoughts?
PS: I think I spotted a number of additional minor issues in your
series, but I won't describe them here, as I hope the simplification
will make most of them go away.
On Tue, Jul 16, 2024 at 08:08:42PM +0200, Markus Armbruster wrote:
> Sorry for the delay; too many distractions, and I needed a good think.
>
> Daniel P. Berrangé <berrange@redhat.com> writes:
>
> > On Fri, Jul 12, 2024 at 10:50:54AM +0200, Markus Armbruster wrote:
> >> Daniel P. Berrangé <berrange@redhat.com> writes:
> >>
> >> > On Fri, Jul 12, 2024 at 10:07:34AM +0200, Markus Armbruster wrote:
> >> >> Daniel P. Berrangé <berrange@redhat.com> writes:
> >> >>
> >> >> > The 'command-features' pragma allows for defining additional
> >> >> > special features that are unique to a particular QAPI schema
> >> >> > instance and its implementation.
> >> >>
> >> >> So far, we have special features (predefined, known to the generator and
> >> >> treated specially), and normal features (user-defined, not known to the
> >> >> generator). You create a new kind in between: user-defined, not known
> >> >> to the generator, yet treated specially (I guess?). Hmm.
> >> >>
> >> >> Could you at least hint at indented use here? What special treatment do
> >> >> you have in mind?
> >> >
> >> > Essentially, these features are a way to attach metadata to commands that
> >> > the server side impl can later query. This eliminates the need to hardcode
> >> > lists of commands, such as in QGA which hardcodes a list of commands which
> >> > are safe to use when filesystems are frozen. This is illustrated later in
> >> > this series.
> >>
> >> Please update docs/devel/qapi-code-gen.rst section "Pragma directives",
> >> and maybe section "Features".
>
> Second thoughts; see below.
>
> >> I'm not sure conflating the new kind of feature with existing special
> >> features is a good idea. I need to review more of the series before I
> >> can make up my mind.
> >
> > I originally implemented a completely separate 'tags' concept in the
> > QAPI parser, before deciding I was just re-inventing 'features' for
> > no obvious benefit.
> >
> > The other nice thing about using features is that these are exposed
> > in the schema and docs. With the 'fsfreeze' restriction in code,
> > there's no formal docs of what commands are allowed when frozen, and
> > this is also not exposed in QAPI schema to apps. Using 'features'
> > we get all that as standard.
>
> When you need to tack a mark to one or more things for whatever purpose
> *and* expose it to QMP clients, then features make sense. This is the
> case here.
>
> Initially, features were strictly an external interface annotation, and
> were not meant to be used within QEMU. All features were user-defined.
>
> This changed when I created configurable policy for deprecated and
> unstable management interfaces: the policy engine needs to check for
> features 'deprecated' and 'unstable'. Since the policy engine is partly
> implemented in generated code, these two features need to be baked into
> the generator. This makes them special.
>
> You need less than that: a predicate "does <command> have <feature>" for
> certain features, ideally without baking them into the generator.
>
> The command registry already tracks each command's special features for
> use by the policy engine. Obvious idea: also track the features you
> want to pass to the predicate.
>
> Your series adds tracking for exactly the features you need:
>
> * Enumerate them in the schema with new pragma command-features
>
> Missing: documentation for the pragma.
>
> * Generate an extension QapiSpecialFeatureCustom of existing enum
> QapiSpecialFeature, which is not generated. The latter is in
> qapi/util.h, the former in ${prefix}qapi-init-commands.h.
>
> * Mark these features special for commands only, so existing registry
> machinery tracks them. Do *not* make them special elsewhere, because
> that would break things.
>
> Feels like a hack. Minor trap: if you use the same feature in
> multiple schemas, multiple generated headers will define the same enum
> constant, possibly with different values. If you manage to include
> the wrong header *and* the value differs there, you'll likely lose
> hair.
>
> * Missing: tests.
>
> I think we can avoid supplying most of the missing bits. The main QAPI
> schema uses five features: deprecated, unstable,
> allow-write-only-overlay, dynamic-auto-read-only, fdset. The QGA QAPI
> schema uses four, namely the four you add in this series. Why not track
> all features, and dispense with the pragma? Like this:
>
> * Change type of feature bitsets to uint64_t (it's unsigned now).
>
> * Error out if a schema has more than 64 features.
>
> * Move enum QapiSpecialFeature into a new generated header.
>
> * Generate a member for each feature, not just the two predefined ones.
>
> * Pass all command features to the registry, not just the special ones.
>
> * Recommended: do the same elsewhere, i.e. replace
> gen_special_features() by gen_features().
>
> Thoughts?
So basically the code would always have access to all features, and
we would have no notion of "special" features any more.
I'm happy to give that a try.
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 :|
Daniel P. Berrangé <berrange@redhat.com> writes: > So basically the code would always have access to all features, and > we would have no notion of "special" features any more. "Special" gets reduced to "predefined / known to the generator". > I'm happy to give that a try. Thank you!
© 2016 - 2024 Red Hat, Inc.