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

[Marc-André Lureau]

Hi

On Mon, Sep 4, 2017 at 3:27 PM, Markus Armbruster <address@hidden> wrote:
> Marc-André Lureau <address@hidden> 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?

yes

>
>> A following patch for qapi-code-gen.txt will provide more complete
>> documentation for 'if' usage.
>>
>> Signed-off-by: Marc-André Lureau <address@hidden>
>> ---
>>  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.
>

ok, thanks

>> +
>> +
>>  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.

Not all, 'data' is not required for commands & events.

And 'if' is always optional.

But anyway, I modified it now to pass it as argument...

>
>> @@ -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.
>

That's a follow-up patch "qapi: add 'ifcond' to visitor methods"

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



-- 
Marc-André Lureau