Re: [PATCH 01/13] qapi: introduce 'runtime_if' for QAPI json

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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