Re: [Qemu-devel] [PATCH v6 18/27] qapi: add an error in case a discriminator is conditionnal
Hi
On Thu, Dec 6, 2018 at 8:25 PM Markus Armbruster wrote:
>
> Marc-André Lureau 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
> > ---
> > 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 00..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
Re: [Qemu-devel] [PATCH v6 18/27] qapi: add an error in case a discriminator is conditionnal
One more thing: in the subject, s/conditionnal/conditional/.
Re: [Qemu-devel] [PATCH v6 18/27] qapi: add an error in case a discriminator is conditionnal
Marc-André Lureau 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?
>Instead,
> the union could be made conditional.
What do you mean by that?
> Signed-off-by: Marc-André Lureau
> ---
> 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:
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 00..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 00..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
[Qemu-devel] [PATCH v6 18/27] qapi: add an error in case a discriminator is conditionnal
Making a discriminator conditonal doesn't make much sense. Instead,
the union could be made conditional.
Signed-off-by: Marc-André Lureau
---
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'])
@@ -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 00..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 00..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 00..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 00..e69de29bb2
--
2.18.0.rc1
