| Message ID | 20240604153242.251334-5-berrange@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Improve mechanism for configuring allowed commands | expand |
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
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
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
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!
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:
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(+)