Re: [Qemu-devel] [PATCH v6 18/27] qapi: add an error in case a discriminator is conditionnal

[Marc-André Lureau]

Hi

On Thu, Dec 6, 2018 at 8:25 PM Markus Armbruster <address@hidden> wrote:
>
> Marc-André Lureau <address@hidden> writes:
>
> > Making a discriminator conditonal doesn't make much sense.
>
> Good point (so easy to overlook!), but why first add the 'if' feature
> broken that way in PATCH 16, then fix it up in PATCH 18?

Not sure, I guess I found out after. Feel free to squash

>
> >                                                            Instead,
> > the union could be made conditional.
>
> What do you mean by that?

That the alternative is probably to make the whole union conditional

>
> > Signed-off-by: Marc-André Lureau <address@hidden>
> > ---
> >  scripts/qapi/common.py                          | 11 +++++++++--
> >  tests/Makefile.include                          |  1 +
> >  .../flat-union-invalid-if-discriminator.err     |  1 +
> >  .../flat-union-invalid-if-discriminator.exit    |  1 +
> >  .../flat-union-invalid-if-discriminator.json    | 17 +++++++++++++++++
> >  .../flat-union-invalid-if-discriminator.out     |  0
> >  6 files changed, 29 insertions(+), 2 deletions(-)
> >  create mode 100644 
> > tests/qapi-schema/flat-union-invalid-if-discriminator.err
> >  create mode 100644 
> > tests/qapi-schema/flat-union-invalid-if-discriminator.exit
> >  create mode 100644 
> > tests/qapi-schema/flat-union-invalid-if-discriminator.json
> >  create mode 100644 
> > tests/qapi-schema/flat-union-invalid-if-discriminator.out
> >
> > diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> > index 9b95f8cfe9..13fbb28493 100644
> > --- a/scripts/qapi/common.py
> > +++ b/scripts/qapi/common.py
> > @@ -577,7 +577,8 @@ def find_alternate_member_qtype(qapi_type):
> >
> >  # Return the discriminator enum define if discriminator is specified as an
> >  # enum type, otherwise return None.
> > -def discriminator_find_enum_define(expr):
> > +def discriminator_find_enum_define(expr, info):
> > +    name = expr['union']
> >      base = expr.get('base')
> >      discriminator = expr.get('discriminator')
> >
> > @@ -592,6 +593,11 @@ def discriminator_find_enum_define(expr):
> >      if not discriminator_member:
> >          return None
> >
> > +    if discriminator_member.get('if'):
> > +        raise QAPISemError(info, 'The discriminator %s.%s for union %s '
> > +                           'must not be conditional' %
> > +                           (base, discriminator, name))
> > +
> >      return enum_types.get(discriminator_member['type'])
> >
> >
>
> I'm afraid this isn't the proper place to check.  It's an accessor
> function for @struct_types and @enum_types.  The check should go into
> check_union(), like this:

indeed, thanks for the hint

>
> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> index 0cf39df404..c1bc9e9c29 100644
> --- a/scripts/qapi/common.py
> +++ b/scripts/qapi/common.py
> @@ -799,6 +794,11 @@ def check_union(expr, info):
>                                 "Discriminator '%s' is not a member of base "
>                                 "struct '%s'"
>                                 % (discriminator, base))
> +        if discriminator_member.get('if'):
> +            raise QAPISemError(info, 'The discriminator %s.%s for union %s '
> +                               'must not be conditional' %
> +                               (base, discriminator, name))
> +
>          enum_define = enum_types.get(discriminator_member['type'])
>          allow_metas = ['struct']
>          # Do not allow string discriminator
>
> > @@ -1020,7 +1026,8 @@ def check_exprs(exprs):
> >
> >          if 'include' in expr:
> >              continue
> > -        if 'union' in expr and not discriminator_find_enum_define(expr):
> > +        info = expr_elem['info']
> > +        if 'union' in expr and not discriminator_find_enum_define(expr, 
> > info):
> >              name = '%sKind' % expr['union']
> >          elif 'alternate' in expr:
> >              name = '%sKind' % expr['alternate']
> > diff --git a/tests/Makefile.include b/tests/Makefile.include
> > index 43e100a6cd..abc3fdf764 100644
> > --- a/tests/Makefile.include
> > +++ b/tests/Makefile.include
> > @@ -510,6 +510,7 @@ qapi-schema += flat-union-inline.json
> >  qapi-schema += flat-union-int-branch.json
> >  qapi-schema += flat-union-invalid-branch-key.json
> >  qapi-schema += flat-union-invalid-discriminator.json
> > +qapi-schema += flat-union-invalid-if-discriminator.json
> >  qapi-schema += flat-union-no-base.json
> >  qapi-schema += flat-union-optional-discriminator.json
> >  qapi-schema += flat-union-string-discriminator.json
> > diff --git a/tests/qapi-schema/flat-union-invalid-if-discriminator.err 
> > b/tests/qapi-schema/flat-union-invalid-if-discriminator.err
> > new file mode 100644
> > index 0000000000..0c94c9860d
> > --- /dev/null
> > +++ b/tests/qapi-schema/flat-union-invalid-if-discriminator.err
> > @@ -0,0 +1 @@
> > +tests/qapi-schema/flat-union-invalid-if-discriminator.json:13: The 
> > discriminator TestBase.enum1 for union TestUnion must not be conditional
> > diff --git a/tests/qapi-schema/flat-union-invalid-if-discriminator.exit 
> > b/tests/qapi-schema/flat-union-invalid-if-discriminator.exit
> > new file mode 100644
> > index 0000000000..d00491fd7e
> > --- /dev/null
> > +++ b/tests/qapi-schema/flat-union-invalid-if-discriminator.exit
> > @@ -0,0 +1 @@
> > +1
> > diff --git a/tests/qapi-schema/flat-union-invalid-if-discriminator.json 
> > b/tests/qapi-schema/flat-union-invalid-if-discriminator.json
> > new file mode 100644
> > index 0000000000..618ec36396
> > --- /dev/null
> > +++ b/tests/qapi-schema/flat-union-invalid-if-discriminator.json
> > @@ -0,0 +1,17 @@
> > +{ 'enum': 'TestEnum',
> > +  'data': [ 'value1', 'value2' ] }
> > +
> > +{ 'struct': 'TestBase',
> > +  'data': { 'enum1': { 'type': 'TestEnum', 'if': 'FOO' } } }
> > +
> > +{ 'struct': 'TestTypeA',
> > +  'data': { 'string': 'str' } }
> > +
> > +{ 'struct': 'TestTypeB',
> > +  'data': { 'integer': 'int' } }
> > +
> > +{ 'union': 'TestUnion',
> > +  'base': 'TestBase',
> > +  'discriminator': 'enum1',
> > +  'data': { 'value1': 'TestTypeA',
> > +            'value2': 'TestTypeB' } }
> > diff --git a/tests/qapi-schema/flat-union-invalid-if-discriminator.out 
> > b/tests/qapi-schema/flat-union-invalid-if-discriminator.out
> > new file mode 100644
> > index 0000000000..e69de29bb2
>


--
Marc-André Lureau