Re: [Qemu-devel] [PATCH v2 15/54] qapi: add 'if' to top-level expressions

[Markus Armbruster <armbru@redhat.com>]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Accept 'if' key in top-level elements, accepted as string or list of
> string type. The following patches will modify the test visitor to
> check the value is correctly saved, and generate #if/#endif code (as a
> single #if/endif line or a series for a list).
>
> Example of 'if' key:
> { 'struct': 'TestIfStruct', 'data': { 'foo': 'int' },                                                                                                                                                               'if': 'defined(TEST_IF_STRUCT)' }

Lost line break?

> A following patch for qapi-code-gen.txt will provide more complete
> documentation for 'if' usage.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  scripts/qapi.py                         | 13 +++++++++++++
>  tests/test-qmp-commands.c               |  6 ++++++
>  tests/qapi-schema/qapi-schema-test.json | 20 ++++++++++++++++++++
>  tests/qapi-schema/qapi-schema-test.out  | 22 ++++++++++++++++++++++
>  4 files changed, 61 insertions(+)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 73adb90379..a94d93ada4 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -639,6 +639,16 @@ def add_name(name, info, meta, implicit=False):
>      all_names[name] = meta
>  
>  
> +def check_if(expr, info):
> +    ifcond = expr.get('if')
> +    if not ifcond or isinstance(ifcond, str):
> +        return
> +    if (not isinstance(ifcond, list) or
> +            any([not isinstance(elt, str) for elt in ifcond])):
> +        raise QAPISemError(info, "'if' condition requires a string or "
> +                           "a list of string")

The error also triggers on inputs 'if': '' and 'if': [].

The first one doesn't make sense (the C compiler will surely barf).  We
could leave rejecting it to the C compiler.  If we choose to reject it
here, we need a better error message, because the one above is
misleading.

The second one is a roundabout way to say "unconditional".  If we choose
to reject that, we also need a non-misleading error message.

The error can't trigger on absent 'if', because we don't get called
then.  To make that locally obvious, please say

       ifcond = expr['if']

Moreover, I'd prefer to avoid mixing conditionals with opposite sense,
i.e. if good: return; if bad: raise.

Taken together:

   def check_if(expr, info):
       ifcond = expr['if']
       if isinstance(ifcond, str):
           if ifcond == '':
               raise QAPISemError(info, "'if' condition '' makes no sense")
           return
       if isinstance(ifcond, list):
           if ifcond == []:
               raise QAPISemError(info, "'if' condition [] is useless")
           for elt in ifcond:
               if not isinstance(elt, str):
                   raise QAPISemError(
                           info, "'if' condition %s makes no sense" % elt)
           return
       raise QAPISemError(
               info, "'if' condition must be a string or a list of strings")

Written this way (one case after the other), each error has to report
just one narrow problem.  Makes crafting precise error messages easier.

> +
> +
>  def check_type(info, source, value, allow_array=False,
>                 allow_dict=False, allow_optional=False,
>                 allow_metas=[]):
> @@ -865,6 +875,7 @@ def check_keys(expr_elem, meta, required, optional=[]):
>      expr = expr_elem['expr']
>      info = expr_elem['info']
>      name = expr[meta]
> +    optional = optional + ['if']  # all top-level elem accept if
>      if not isinstance(name, str):
>          raise QAPISemError(info, "'%s' key must have a string value" % meta)
>      required = required + [meta]

All top-level expressions require 'data'.  Done the obvious way: all
callers pass 'data' in @required.  Let's do 'if' the same way for
consistency.

> @@ -880,6 +891,8 @@ def check_keys(expr_elem, meta, required, optional=[]):
>              raise QAPISemError(info,
>                                 "'%s' of %s '%s' should only use true value"
>                                 % (key, meta, name))
> +        if key == 'if':
> +            check_if(expr, info)
>      for key in required:
>          if key not in expr:
>              raise QAPISemError(info, "Key '%s' is missing from %s '%s'"
> diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c
> index 904c89d4d4..9b9a7ffee7 100644
> --- a/tests/test-qmp-commands.c
> +++ b/tests/test-qmp-commands.c
> @@ -10,6 +10,12 @@
>  
>  static QmpCommandList qmp_commands;
>  
> +/* #if defined(TEST_IF_CMD) */
> +void qmp_TestIfCmd(TestIfStruct *foo, Error **errp)
> +{
> +}
> +/* #endif */
> +
>  void qmp_user_def_cmd(Error **errp)
>  {
>  }
> diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
> index c72dbd8050..dc2c444fc1 100644
> --- a/tests/qapi-schema/qapi-schema-test.json
> +++ b/tests/qapi-schema/qapi-schema-test.json
> @@ -188,3 +188,23 @@
>    'data': { 'a': ['__org.qemu_x-Enum'], 'b': ['__org.qemu_x-Struct'],
>              'c': '__org.qemu_x-Union2', 'd': '__org.qemu_x-Alt' },
>    'returns': '__org.qemu_x-Union1' }
> +
> +# test 'if' condition handling
> +
> +{ 'struct': 'TestIfStruct', 'data': { 'foo': 'int' },
> +  'if': 'defined(TEST_IF_STRUCT)' }
> +
> +{ 'enum': 'TestIfEnum', 'data': [ 'foo', 'bar' ],
> +  'if': 'defined(TEST_IF_ENUM)' }
> +
> +{ 'union': 'TestIfUnion', 'data': { 'foo': 'TestStruct' },
> +  'if': 'defined(TEST_IF_UNION) && defined(TEST_IF_STRUCT)' }
> +
> +{ 'alternate': 'TestIfAlternate', 'data': { 'foo': 'int', 'bar': 'TestStruct' },
> +  'if': 'defined(TEST_IF_ALT) && defined(TEST_IF_STRUCT)' }
> +
> +{ 'command': 'TestIfCmd', 'data': { 'foo': 'TestIfStruct' },
> +  'if': 'defined(TEST_IF_CMD) && defined(TEST_IF_STRUCT)' }
> +
> +{ 'event': 'TestIfEvent', 'data': { 'foo': 'TestIfStruct' },
> +  'if': 'defined(TEST_IF_EVT) && defined(TEST_IF_STRUCT)' }
> diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
> index 3b1e9082d3..7fbaea19bc 100644
> --- a/tests/qapi-schema/qapi-schema-test.out
> +++ b/tests/qapi-schema/qapi-schema-test.out
> @@ -52,6 +52,22 @@ enum QEnumTwo ['value1', 'value2']
>      prefix QENUM_TWO
>  enum QType ['none', 'qnull', 'qnum', 'qstring', 'qdict', 'qlist', 'qbool']
>      prefix QTYPE
> +alternate TestIfAlternate
> +    tag type
> +    case foo: int
> +    case bar: TestStruct
> +command TestIfCmd q_obj_TestIfCmd-arg -> None
> +   gen=True success_response=True boxed=False
> +enum TestIfEnum ['foo', 'bar']
> +event TestIfEvent q_obj_TestIfEvent-arg
> +   boxed=False
> +object TestIfStruct
> +    member foo: int optional=False
> +object TestIfUnion
> +    member type: TestIfUnionKind optional=False
> +    tag type
> +    case foo: q_obj_TestStruct-wrapper
> +enum TestIfUnionKind ['foo']
>  object TestStruct
>      member integer: int optional=False
>      member boolean: bool optional=False
> @@ -172,6 +188,12 @@ object q_obj_EVENT_D-arg
>      member b: str optional=False
>      member c: str optional=True
>      member enum3: EnumOne optional=True
> +object q_obj_TestIfCmd-arg
> +    member foo: TestIfStruct optional=False
> +object q_obj_TestIfEvent-arg
> +    member foo: TestIfStruct optional=False
> +object q_obj_TestStruct-wrapper
> +    member data: TestStruct optional=False
>  object q_obj_UserDefFlatUnion2-base
>      member integer: int optional=True
>      member string: str optional=False

The conditionals aren't visible in qapi-schema-test.out.  They should
be.

*Much* easier to review than its predecessor PATCH 07/26.  Appreciated!