| Message ID | 20250507231442.879619-2-pierrick.bouvier@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | single-binary: make QAPI generated files common | expand |
On 8/5/25 01:14, Pierrick Bouvier wrote: > This new entry can be used in QAPI json to specify a runtime conditional > to expose any entry, similar to existing 'if', that applies at compile > time, thanks to ifdef. The element is always defined in C, but not > exposed through the schema and visit functions, thus being hidden for a > QMP consumer. > > QAPISchemaIfCond is extended to parse this information. A first version > was tried duplicating this, but this proved to be much more boilerplate > than needed to pass information through all function calls. > > 'if' and 'runtime_if' can be combined elegantly on a single item, > allowing to restrict an element to be present based on compile time > defines, and runtime checks at the same time. > > Note: This commit only adds parsing of runtime_if, and does not hide > anything yet. > > For review: > > - I don't really like "runtime_if" name. > What would make sense, IMHO, is to rename existing 'if' to 'ifdef', > and reuse 'if' for 'runtime_if'. Since it requires invasive changes, I > would prefer to get agreement before wasting time in case you prefer > any other naming convention. Let me know what you'd like. Or rename 'if' as 'buildtime_if'. /s! > > - As mentioned in second paragraph, I think our best implementation > would be to extend existing QAPISchemaIfCond, as it's really > complicated to extend all call sites if we have another new object. > > - No tests/doc added at this time, as I prefer to wait that we decide > about naming and proposed approach first. > > Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> > --- > scripts/qapi/common.py | 16 +++++++++++- > scripts/qapi/expr.py | 9 ++++--- > scripts/qapi/gen.py | 56 +++++++++++++++++++++++++++++++++++++++++- > scripts/qapi/schema.py | 44 ++++++++++++++++++++++++--------- > 4 files changed, 107 insertions(+), 18 deletions(-) > > diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py > index d7c8aa3365c..0e8e2abeb58 100644 > --- a/scripts/qapi/common.py > +++ b/scripts/qapi/common.py > @@ -229,6 +229,8 @@ def gen_infix(operator: str, operands: Sequence[Any]) -> str: > def cgen_ifcond(ifcond: Optional[Union[str, Dict[str, Any]]]) -> str: > return gen_ifcond(ifcond, 'defined(%s)', '!%s', ' && ', ' || ') > > +def cgen_runtime_ifcond(ifcond: Optional[Union[str, Dict[str, Any]]]) -> str: > + return gen_ifcond(ifcond, '%s', '!%s', ' && ', ' || ') > > def docgen_ifcond(ifcond: Optional[Union[str, Dict[str, Any]]]) -> str: > # TODO Doc generated for conditions needs polish > @@ -242,7 +244,6 @@ def gen_if(cond: str) -> str: > #if %(cond)s > ''', cond=cond) > > - > def gen_endif(cond: str) -> str: > if not cond: > return '' > @@ -250,6 +251,19 @@ def gen_endif(cond: str) -> str: > #endif /* %(cond)s */ > ''', cond=cond) > > +def gen_runtime_if(cond: str) -> str: > + if not cond: > + return '' > + return mcgen(''' > +if (%(cond)s) { > +''', cond=cond) > + > +def gen_runtime_endif(cond: str) -> str: > + if not cond: > + return '' > + return mcgen(''' > +} /* (%(cond)s) */ No need for extra parenthesis in comment: +} /* %(cond)s */ > +''', cond=cond) > > def must_match(pattern: str, string: str) -> Match[str]: > match = re.match(pattern, string) > diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py > index cae0a083591..5ae26395964 100644 > --- a/scripts/qapi/expr.py > +++ b/scripts/qapi/expr.py > @@ -392,7 +392,8 @@ def check_type_implicit(value: Optional[object], > permit_underscore=permissive) > if c_name(key, False) == 'u' or c_name(key, False).startswith('has_'): > raise QAPISemError(info, "%s uses reserved name" % key_source) > - check_keys(arg, info, key_source, ['type'], ['if', 'features']) > + check_keys(arg, info, key_source, ['type'], ['if', 'features', > + 'runtime_if']) > check_if(arg, info, key_source) > check_features(arg.get('features'), info) > check_type_name_or_array(arg['type'], info, key_source) > @@ -642,7 +643,7 @@ def check_exprs(exprs: List[QAPIExpression]) -> List[QAPIExpression]: > elif meta == 'union': > check_keys(expr, info, meta, > ['union', 'base', 'discriminator', 'data'], > - ['if', 'features']) > + ['if', 'runtime_if', 'features']) > normalize_members(expr.get('base')) > normalize_members(expr['data']) > check_union(expr) > @@ -659,8 +660,8 @@ def check_exprs(exprs: List[QAPIExpression]) -> List[QAPIExpression]: > elif meta == 'command': > check_keys(expr, info, meta, > ['command'], > - ['data', 'returns', 'boxed', 'if', 'features', > - 'gen', 'success-response', 'allow-oob', > + ['data', 'returns', 'boxed', 'if', 'runtime_if', > + 'features', 'gen', 'success-response', 'allow-oob', > 'allow-preconfig', 'coroutine']) > normalize_members(expr.get('data')) > check_command(expr) Why can't we merge here the changes from patch 9? -- >8 -- diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py index 5ae26395964..f31f28ecb10 100644 --- a/scripts/qapi/expr.py +++ b/scripts/qapi/expr.py @@ -638,7 +638,8 @@ def check_exprs(exprs: List[QAPIExpression]) -> List[QAPIExpression]: if meta == 'enum': check_keys(expr, info, meta, - ['enum', 'data'], ['if', 'features', 'prefix']) + ['enum', 'data'], ['if', 'runtime_if', 'features', + 'prefix']) check_enum(expr) elif meta == 'union': check_keys(expr, info, meta, @@ -654,7 +655,8 @@ def check_exprs(exprs: List[QAPIExpression]) -> List[QAPIExpression]: check_alternate(expr) elif meta == 'struct': check_keys(expr, info, meta, - ['struct', 'data'], ['base', 'if', 'features']) + ['struct', 'data'], ['base', 'if', 'runtime_if', + 'features']) normalize_members(expr['data']) check_struct(expr) elif meta == 'command': @@ -667,7 +669,8 @@ def check_exprs(exprs: List[QAPIExpression]) -> List[QAPIExpression]: check_command(expr) elif meta == 'event': check_keys(expr, info, meta, - ['event'], ['data', 'boxed', 'if', 'features']) + ['event'], ['data', 'boxed', 'if', 'runtime_if', + 'features']) normalize_members(expr.get('data')) check_event(expr) else: --- Otherwise, patch LGTM :)
On 5/7/25 11:53 PM, Philippe Mathieu-Daudé wrote: > On 8/5/25 01:14, Pierrick Bouvier wrote: >> This new entry can be used in QAPI json to specify a runtime conditional >> to expose any entry, similar to existing 'if', that applies at compile >> time, thanks to ifdef. The element is always defined in C, but not >> exposed through the schema and visit functions, thus being hidden for a >> QMP consumer. >> >> QAPISchemaIfCond is extended to parse this information. A first version >> was tried duplicating this, but this proved to be much more boilerplate >> than needed to pass information through all function calls. >> >> 'if' and 'runtime_if' can be combined elegantly on a single item, >> allowing to restrict an element to be present based on compile time >> defines, and runtime checks at the same time. >> >> Note: This commit only adds parsing of runtime_if, and does not hide >> anything yet. >> >> For review: >> >> - I don't really like "runtime_if" name. >> What would make sense, IMHO, is to rename existing 'if' to 'ifdef', >> and reuse 'if' for 'runtime_if'. Since it requires invasive changes, I >> would prefer to get agreement before wasting time in case you prefer >> any other naming convention. Let me know what you'd like. > > Or rename 'if' as 'buildtime_if'. /s! > I'll let Markus do the bikeshed as he's the maintainer and the one who may finally (or not) merge this. >> >> - As mentioned in second paragraph, I think our best implementation >> would be to extend existing QAPISchemaIfCond, as it's really >> complicated to extend all call sites if we have another new object. >> >> - No tests/doc added at this time, as I prefer to wait that we decide >> about naming and proposed approach first. >> >> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> >> --- >> scripts/qapi/common.py | 16 +++++++++++- >> scripts/qapi/expr.py | 9 ++++--- >> scripts/qapi/gen.py | 56 +++++++++++++++++++++++++++++++++++++++++- >> scripts/qapi/schema.py | 44 ++++++++++++++++++++++++--------- >> 4 files changed, 107 insertions(+), 18 deletions(-) >> >> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py >> index d7c8aa3365c..0e8e2abeb58 100644 >> --- a/scripts/qapi/common.py >> +++ b/scripts/qapi/common.py >> @@ -229,6 +229,8 @@ def gen_infix(operator: str, operands: Sequence[Any]) -> str: >> def cgen_ifcond(ifcond: Optional[Union[str, Dict[str, Any]]]) -> str: >> return gen_ifcond(ifcond, 'defined(%s)', '!%s', ' && ', ' || ') >> >> +def cgen_runtime_ifcond(ifcond: Optional[Union[str, Dict[str, Any]]]) -> str: >> + return gen_ifcond(ifcond, '%s', '!%s', ' && ', ' || ') >> >> def docgen_ifcond(ifcond: Optional[Union[str, Dict[str, Any]]]) -> str: >> # TODO Doc generated for conditions needs polish >> @@ -242,7 +244,6 @@ def gen_if(cond: str) -> str: >> #if %(cond)s >> ''', cond=cond) >> >> - >> def gen_endif(cond: str) -> str: >> if not cond: >> return '' >> @@ -250,6 +251,19 @@ def gen_endif(cond: str) -> str: >> #endif /* %(cond)s */ >> ''', cond=cond) >> >> +def gen_runtime_if(cond: str) -> str: >> + if not cond: >> + return '' >> + return mcgen(''' >> +if (%(cond)s) { >> +''', cond=cond) >> + >> +def gen_runtime_endif(cond: str) -> str: >> + if not cond: >> + return '' >> + return mcgen(''' >> +} /* (%(cond)s) */ > > No need for extra parenthesis in comment: > > +} /* %(cond)s */ > >> +''', cond=cond) >> >> def must_match(pattern: str, string: str) -> Match[str]: >> match = re.match(pattern, string) >> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py >> index cae0a083591..5ae26395964 100644 >> --- a/scripts/qapi/expr.py >> +++ b/scripts/qapi/expr.py >> @@ -392,7 +392,8 @@ def check_type_implicit(value: Optional[object], >> permit_underscore=permissive) >> if c_name(key, False) == 'u' or c_name(key, False).startswith('has_'): >> raise QAPISemError(info, "%s uses reserved name" % key_source) >> - check_keys(arg, info, key_source, ['type'], ['if', 'features']) >> + check_keys(arg, info, key_source, ['type'], ['if', 'features', >> + 'runtime_if']) >> check_if(arg, info, key_source) >> check_features(arg.get('features'), info) >> check_type_name_or_array(arg['type'], info, key_source) >> @@ -642,7 +643,7 @@ def check_exprs(exprs: List[QAPIExpression]) -> List[QAPIExpression]: >> elif meta == 'union': >> check_keys(expr, info, meta, >> ['union', 'base', 'discriminator', 'data'], >> - ['if', 'features']) >> + ['if', 'runtime_if', 'features']) >> normalize_members(expr.get('base')) >> normalize_members(expr['data']) >> check_union(expr) >> @@ -659,8 +660,8 @@ def check_exprs(exprs: List[QAPIExpression]) -> List[QAPIExpression]: >> elif meta == 'command': >> check_keys(expr, info, meta, >> ['command'], >> - ['data', 'returns', 'boxed', 'if', 'features', >> - 'gen', 'success-response', 'allow-oob', >> + ['data', 'returns', 'boxed', 'if', 'runtime_if', >> + 'features', 'gen', 'success-response', 'allow-oob', >> 'allow-preconfig', 'coroutine']) >> normalize_members(expr.get('data')) >> check_command(expr) > > Why can't we merge here the changes from patch 9? > Oops, that's a rebase mistake, thanks. It belongs here indeed. > -- >8 -- > diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py > index 5ae26395964..f31f28ecb10 100644 > --- a/scripts/qapi/expr.py > +++ b/scripts/qapi/expr.py > @@ -638,7 +638,8 @@ def check_exprs(exprs: List[QAPIExpression]) -> > List[QAPIExpression]: > > if meta == 'enum': > check_keys(expr, info, meta, > - ['enum', 'data'], ['if', 'features', 'prefix']) > + ['enum', 'data'], ['if', 'runtime_if', 'features', > + 'prefix']) > check_enum(expr) > elif meta == 'union': > check_keys(expr, info, meta, > @@ -654,7 +655,8 @@ def check_exprs(exprs: List[QAPIExpression]) -> > List[QAPIExpression]: > check_alternate(expr) > elif meta == 'struct': > check_keys(expr, info, meta, > - ['struct', 'data'], ['base', 'if', 'features']) > + ['struct', 'data'], ['base', 'if', 'runtime_if', > + 'features']) > normalize_members(expr['data']) > check_struct(expr) > elif meta == 'command': > @@ -667,7 +669,8 @@ def check_exprs(exprs: List[QAPIExpression]) -> > List[QAPIExpression]: > check_command(expr) > elif meta == 'event': > check_keys(expr, info, meta, > - ['event'], ['data', 'boxed', 'if', 'features']) > + ['event'], ['data', 'boxed', 'if', 'runtime_if', > + 'features']) > normalize_members(expr.get('data')) > check_event(expr) > else: > --- > > Otherwise, patch LGTM :)
Consensus is to shelve this series, and eliminate target-specific conditionals instead. But let me scribble down a few notes for posterity just in case we ever take it off the shelf again. Pierrick Bouvier <pierrick.bouvier@linaro.org> writes: > This new entry can be used in QAPI json to specify a runtime conditional > to expose any entry, similar to existing 'if', that applies at compile > time, thanks to ifdef. The element is always defined in C, but not > exposed through the schema and visit functions, thus being hidden for a > QMP consumer. > > QAPISchemaIfCond is extended to parse this information. A first version > was tried duplicating this, but this proved to be much more boilerplate > than needed to pass information through all function calls. > > 'if' and 'runtime_if' can be combined elegantly on a single item, > allowing to restrict an element to be present based on compile time > defines, and runtime checks at the same time. I understand the combination is "and", i.e. both conditions need to be satisfied. The syntax change I'd consider elegant (it's subjective!) is *none*. Instead of 'if': 'CONFIG_DINGS', 'runtime_if': 'target_bums()' use 'if': ['all': ['CONFIG_DINGS', 'target_bums()']] Might need semantic restrictions to simplify the implementation. > Note: This commit only adds parsing of runtime_if, and does not hide > anything yet. > > For review: > > - I don't really like "runtime_if" name. > What would make sense, IMHO, is to rename existing 'if' to 'ifdef', > and reuse 'if' for 'runtime_if'. Since it requires invasive changes, I > would prefer to get agreement before wasting time in case you prefer > any other naming convention. Let me know what you'd like. > > - As mentioned in second paragraph, I think our best implementation > would be to extend existing QAPISchemaIfCond, as it's really > complicated to extend all call sites if we have another new object. I figure the alternative is an abstract type with two concrete subtypes, one for each kind of conditional. > - No tests/doc added at this time, as I prefer to wait that we decide > about naming and proposed approach first. We'd need * Positive test(s) in tests/qapi-schema/qapi-schema-test.json * Negative tests similar to the ones with have for 'if' * Documentation update docs/devel/qapi-code-gen.rst > Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
On 5/14/25 9:39 PM, Markus Armbruster wrote: > Consensus is to shelve this series, and eliminate target-specific > conditionals instead. But let me scribble down a few notes for > posterity just in case we ever take it off the shelf again. > > Pierrick Bouvier <pierrick.bouvier@linaro.org> writes: > >> This new entry can be used in QAPI json to specify a runtime conditional >> to expose any entry, similar to existing 'if', that applies at compile >> time, thanks to ifdef. The element is always defined in C, but not >> exposed through the schema and visit functions, thus being hidden for a >> QMP consumer. >> >> QAPISchemaIfCond is extended to parse this information. A first version >> was tried duplicating this, but this proved to be much more boilerplate >> than needed to pass information through all function calls. >> >> 'if' and 'runtime_if' can be combined elegantly on a single item, >> allowing to restrict an element to be present based on compile time >> defines, and runtime checks at the same time. > > I understand the combination is "and", i.e. both conditions need to be > satisfied. > Yes, if results in introduction of an ifdef, and runtime_if, an if. #ifdef IF_CONDITION if (runtime_if_condition()) { ... } #endif > The syntax change I'd consider elegant (it's subjective!) is *none*. > Instead of > > 'if': 'CONFIG_DINGS', > 'runtime_if': 'target_bums()' > > use > > 'if': ['all': ['CONFIG_DINGS', 'target_bums()']] > Why not, but I don't see how to identify what would be an ifdef, vs what is an if, and I don't think that using something like "is_capital_letters" or "has_parentheses" is a good thing. > Might need semantic restrictions to simplify the implementation. > >> Note: This commit only adds parsing of runtime_if, and does not hide >> anything yet. >> >> For review: >> >> - I don't really like "runtime_if" name. >> What would make sense, IMHO, is to rename existing 'if' to 'ifdef', >> and reuse 'if' for 'runtime_if'. Since it requires invasive changes, I >> would prefer to get agreement before wasting time in case you prefer >> any other naming convention. Let me know what you'd like. >> >> - As mentioned in second paragraph, I think our best implementation >> would be to extend existing QAPISchemaIfCond, as it's really >> complicated to extend all call sites if we have another new object. > > I figure the alternative is an abstract type with two concrete subtypes, > one for each kind of conditional. > >> - No tests/doc added at this time, as I prefer to wait that we decide >> about naming and proposed approach first. > > We'd need > > * Positive test(s) in tests/qapi-schema/qapi-schema-test.json > > * Negative tests similar to the ones with have for 'if' > > * Documentation update docs/devel/qapi-code-gen.rst > As we decided to follow the approach (1) "Drop target-specific conditionals", I will focus on the other series, and drop this one. Thanks, Pierrick
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index d7c8aa3365c..0e8e2abeb58 100644 --- a/scripts/qapi/common.py +++ b/scripts/qapi/common.py @@ -229,6 +229,8 @@ def gen_infix(operator: str, operands: Sequence[Any]) -> str: def cgen_ifcond(ifcond: Optional[Union[str, Dict[str, Any]]]) -> str: return gen_ifcond(ifcond, 'defined(%s)', '!%s', ' && ', ' || ') +def cgen_runtime_ifcond(ifcond: Optional[Union[str, Dict[str, Any]]]) -> str: + return gen_ifcond(ifcond, '%s', '!%s', ' && ', ' || ') def docgen_ifcond(ifcond: Optional[Union[str, Dict[str, Any]]]) -> str: # TODO Doc generated for conditions needs polish @@ -242,7 +244,6 @@ def gen_if(cond: str) -> str: #if %(cond)s ''', cond=cond) - def gen_endif(cond: str) -> str: if not cond: return '' @@ -250,6 +251,19 @@ def gen_endif(cond: str) -> str: #endif /* %(cond)s */ ''', cond=cond) +def gen_runtime_if(cond: str) -> str: + if not cond: + return '' + return mcgen(''' +if (%(cond)s) { +''', cond=cond) + +def gen_runtime_endif(cond: str) -> str: + if not cond: + return '' + return mcgen(''' +} /* (%(cond)s) */ +''', cond=cond) def must_match(pattern: str, string: str) -> Match[str]: match = re.match(pattern, string) diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py index cae0a083591..5ae26395964 100644 --- a/scripts/qapi/expr.py +++ b/scripts/qapi/expr.py @@ -392,7 +392,8 @@ def check_type_implicit(value: Optional[object], permit_underscore=permissive) if c_name(key, False) == 'u' or c_name(key, False).startswith('has_'): raise QAPISemError(info, "%s uses reserved name" % key_source) - check_keys(arg, info, key_source, ['type'], ['if', 'features']) + check_keys(arg, info, key_source, ['type'], ['if', 'features', + 'runtime_if']) check_if(arg, info, key_source) check_features(arg.get('features'), info) check_type_name_or_array(arg['type'], info, key_source) @@ -642,7 +643,7 @@ def check_exprs(exprs: List[QAPIExpression]) -> List[QAPIExpression]: elif meta == 'union': check_keys(expr, info, meta, ['union', 'base', 'discriminator', 'data'], - ['if', 'features']) + ['if', 'runtime_if', 'features']) normalize_members(expr.get('base')) normalize_members(expr['data']) check_union(expr) @@ -659,8 +660,8 @@ def check_exprs(exprs: List[QAPIExpression]) -> List[QAPIExpression]: elif meta == 'command': check_keys(expr, info, meta, ['command'], - ['data', 'returns', 'boxed', 'if', 'features', - 'gen', 'success-response', 'allow-oob', + ['data', 'returns', 'boxed', 'if', 'runtime_if', + 'features', 'gen', 'success-response', 'allow-oob', 'allow-preconfig', 'coroutine']) normalize_members(expr.get('data')) check_command(expr) diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py index d3c56d45c89..5082eb331f4 100644 --- a/scripts/qapi/gen.py +++ b/scripts/qapi/gen.py @@ -109,6 +109,37 @@ def _wrap_ifcond(ifcond: QAPISchemaIfCond, before: str, after: str) -> str: return out +def _wrap_runtime_ifcond(ifcond: QAPISchemaIfCond, before: str, after: str) -> str: + if before == after: + return after # suppress empty #if ... #endif + + assert after.startswith(before) + out = before + added = after[len(before):] + if added[0] == '\n': + out += '\n' + added = added[1:] + out += ifcond.gen_runtime_if() + out += added + out += ifcond.gen_runtime_endif() + return out + + +def _wrap_runtime_ifcond(ifcond: QAPISchemaIfCond, before: str, after: str) -> str: + if before == after: + return after # suppress empty #if ... #endif + + assert after.startswith(before) + out = before + added = after[len(before):] + if added[0] == '\n': + out += '\n' + added = added[1:] + out += ifcond.gen_runtime_if() + out += added + out += ifcond.gen_runtime_endif() + return out + def build_params(arg_type: Optional[QAPISchemaObjectType], boxed: bool, extra: Optional[str] = None) -> str: @@ -137,12 +168,17 @@ class QAPIGenCCode(QAPIGen): def __init__(self, fname: str): super().__init__(fname) self._start_if: Optional[Tuple[QAPISchemaIfCond, str, str]] = None + self._start_runtime_if: Optional[Tuple[QAPISchemaIfCond, str, str]] = None def start_if(self, ifcond: QAPISchemaIfCond) -> None: assert self._start_if is None self._start_if = (ifcond, self._body, self._preamble) - def end_if(self) -> None: + def start_runtime_if(self, ifcond: QAPISchemaIfCond) -> None: + assert self._start_runtime_if is None + self._start_runtime_if = (ifcond, self._body, self._preamble) + + def end_if(self, runtime: bool = False) -> None: assert self._start_if is not None self._body = _wrap_ifcond(self._start_if[0], self._start_if[1], self._body) @@ -150,8 +186,18 @@ def end_if(self) -> None: self._start_if[2], self._preamble) self._start_if = None + def end_runtime_if(self, runtime: bool = False) -> None: + assert self._start_runtime_if is not None + self._body = _wrap_runtime_ifcond(self._start_runtime_if[0], + self._start_runtime_if[1], self._body) + self._preamble = _wrap_runtime_ifcond(self._start_runtime_if[0], + self._start_runtime_if[2], + self._preamble) + self._start_runtime_if = None + def get_content(self) -> str: assert self._start_if is None + assert self._start_runtime_if is None return super().get_content() @@ -231,6 +277,14 @@ def ifcontext(ifcond: QAPISchemaIfCond, *args: QAPIGenCCode) -> Iterator[None]: for arg in args: arg.end_if() +@contextmanager +def runtime_ifcontext(ifcond: QAPISchemaIfCond, + *args: QAPIGenCCode) -> Iterator[None]: + for arg in args: + arg.start_runtime_if(ifcond) + yield + for arg in args: + arg.end_runtime_if() class QAPISchemaMonolithicCVisitor(QAPISchemaVisitor): def __init__(self, diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index cbe3b5aa91e..533d0dfe088 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -36,9 +36,12 @@ POINTER_SUFFIX, c_name, cgen_ifcond, + cgen_runtime_ifcond, docgen_ifcond, - gen_endif, gen_if, + gen_endif, + gen_runtime_if, + gen_runtime_endif, ) from .error import QAPIError, QAPISemError, QAPISourceError from .expr import check_exprs @@ -50,8 +53,10 @@ class QAPISchemaIfCond: def __init__( self, ifcond: Optional[Union[str, Dict[str, object]]] = None, + runtime_ifcond: Optional[Union[str, Dict[str, object]]] = None, ) -> None: self.ifcond = ifcond + self.runtime_ifcond = runtime_ifcond def _cgen(self) -> str: return cgen_ifcond(self.ifcond) @@ -65,8 +70,17 @@ def gen_endif(self) -> str: def docgen(self) -> str: return docgen_ifcond(self.ifcond) + def _cgen_runtime(self) -> str: + return cgen_runtime_ifcond(self.runtime_ifcond) + + def gen_runtime_if(self) -> str: + return gen_runtime_if(self._cgen_runtime()) + + def gen_runtime_endif(self) -> str: + return gen_runtime_endif(self._cgen_runtime()) + def is_present(self) -> bool: - return bool(self.ifcond) + return bool(self.ifcond) or bool(self.runtime_ifcond) class QAPISchemaEntity: @@ -1281,13 +1295,15 @@ def _make_features( self._feature_dict[feat.name] = feat return [QAPISchemaFeature(f['name'], info, - QAPISchemaIfCond(f.get('if'))) + QAPISchemaIfCond(f.get('if'), + f.get('runtime_if'))) for f in features] def _make_enum_member( self, name: str, ifcond: Optional[Union[str, Dict[str, Any]]], + runtime_ifcond: Optional[Union[str, Dict[str, Any]]], features: Optional[List[Dict[str, Any]]], info: Optional[QAPISourceInfo], ) -> QAPISchemaEnumMember: @@ -1299,6 +1315,7 @@ def _make_enum_members( self, values: List[Dict[str, Any]], info: Optional[QAPISourceInfo] ) -> List[QAPISchemaEnumMember]: return [self._make_enum_member(v['name'], v.get('if'), + v.get('runtime_if'), v.get('features'), info) for v in values] @@ -1338,7 +1355,7 @@ def _def_enum_type(self, expr: QAPIExpression) -> None: name = expr['enum'] data = expr['data'] prefix = expr.get('prefix') - ifcond = QAPISchemaIfCond(expr.get('if')) + ifcond = QAPISchemaIfCond(expr.get('if'), expr.get('runtime_if')) info = expr.info features = self._make_features(expr.get('features'), info) self._def_definition(QAPISchemaEnumType( @@ -1369,7 +1386,8 @@ def _make_members( info: QAPISourceInfo, ) -> List[QAPISchemaObjectTypeMember]: return [self._make_member(key, value['type'], - QAPISchemaIfCond(value.get('if')), + QAPISchemaIfCond(value.get('if'), + value.get('runtime_if')), value.get('features'), info) for (key, value) in data.items()] @@ -1378,7 +1396,7 @@ def _def_struct_type(self, expr: QAPIExpression) -> None: base = expr.get('base') data = expr['data'] info = expr.info - ifcond = QAPISchemaIfCond(expr.get('if')) + ifcond = QAPISchemaIfCond(expr.get('if'), expr.get('runtime_if')) features = self._make_features(expr.get('features'), info) self._def_definition(QAPISchemaObjectType( name, info, expr.doc, ifcond, features, base, @@ -1404,7 +1422,7 @@ def _def_union_type(self, expr: QAPIExpression) -> None: data = expr['data'] assert isinstance(data, dict) info = expr.info - ifcond = QAPISchemaIfCond(expr.get('if')) + ifcond = QAPISchemaIfCond(expr.get('if'), expr.get('runtime_if')) features = self._make_features(expr.get('features'), info) if isinstance(base, dict): base = self._make_implicit_object_type( @@ -1412,7 +1430,8 @@ def _def_union_type(self, expr: QAPIExpression) -> None: 'base', self._make_members(base, info)) variants = [ self._make_variant(key, value['type'], - QAPISchemaIfCond(value.get('if')), + QAPISchemaIfCond(value.get('if'), + value.get('runtime_if')), info) for (key, value) in data.items()] members: List[QAPISchemaObjectTypeMember] = [] @@ -1426,12 +1445,13 @@ def _def_alternate_type(self, expr: QAPIExpression) -> None: name = expr['alternate'] data = expr['data'] assert isinstance(data, dict) - ifcond = QAPISchemaIfCond(expr.get('if')) + ifcond = QAPISchemaIfCond(expr.get('if'), expr.get('runtime_if')) info = expr.info features = self._make_features(expr.get('features'), info) variants = [ self._make_variant(key, value['type'], - QAPISchemaIfCond(value.get('if')), + QAPISchemaIfCond(value.get('if'), + value.get('runtime_if')), info) for (key, value) in data.items()] tag_member = QAPISchemaObjectTypeMember('type', info, 'QType', False) @@ -1450,7 +1470,7 @@ def _def_command(self, expr: QAPIExpression) -> None: allow_oob = expr.get('allow-oob', False) allow_preconfig = expr.get('allow-preconfig', False) coroutine = expr.get('coroutine', False) - ifcond = QAPISchemaIfCond(expr.get('if')) + ifcond = QAPISchemaIfCond(expr.get('if'), expr.get('runtime_if')) info = expr.info features = self._make_features(expr.get('features'), info) if isinstance(data, dict): @@ -1469,7 +1489,7 @@ def _def_event(self, expr: QAPIExpression) -> None: name = expr['event'] data = expr.get('data') boxed = expr.get('boxed', False) - ifcond = QAPISchemaIfCond(expr.get('if')) + ifcond = QAPISchemaIfCond(expr.get('if'), expr.get('runtime_if')) info = expr.info features = self._make_features(expr.get('features'), info) if isinstance(data, dict):
This new entry can be used in QAPI json to specify a runtime conditional to expose any entry, similar to existing 'if', that applies at compile time, thanks to ifdef. The element is always defined in C, but not exposed through the schema and visit functions, thus being hidden for a QMP consumer. QAPISchemaIfCond is extended to parse this information. A first version was tried duplicating this, but this proved to be much more boilerplate than needed to pass information through all function calls. 'if' and 'runtime_if' can be combined elegantly on a single item, allowing to restrict an element to be present based on compile time defines, and runtime checks at the same time. Note: This commit only adds parsing of runtime_if, and does not hide anything yet. For review: - I don't really like "runtime_if" name. What would make sense, IMHO, is to rename existing 'if' to 'ifdef', and reuse 'if' for 'runtime_if'. Since it requires invasive changes, I would prefer to get agreement before wasting time in case you prefer any other naming convention. Let me know what you'd like. - As mentioned in second paragraph, I think our best implementation would be to extend existing QAPISchemaIfCond, as it's really complicated to extend all call sites if we have another new object. - No tests/doc added at this time, as I prefer to wait that we decide about naming and proposed approach first. Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> --- scripts/qapi/common.py | 16 +++++++++++- scripts/qapi/expr.py | 9 ++++--- scripts/qapi/gen.py | 56 +++++++++++++++++++++++++++++++++++++++++- scripts/qapi/schema.py | 44 ++++++++++++++++++++++++--------- 4 files changed, 107 insertions(+), 18 deletions(-)