Making a discriminator conditional doesn't make much sense.  "qapi:
add a dictionary form for TYPE" allows it, this patch fixes
it. Eventually the two could be merged, but for clarity of review it
is kept separate.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 scripts/qapi/common.py                          |  4 ++++
 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, 24 insertions(+)
 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 cc2842bcc7..8ea1dde03b 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -784,6 +784,10 @@ def check_union(expr, info):
                                "Discriminator '%s' is not a member of base "
                                "struct '%s'"
                                % (discriminator, base))
+        if discriminator_value.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_value['type'])
         allow_metas = ['struct']
         # Do not allow string discriminator
diff --git a/tests/Makefile.include b/tests/Makefile.include
index ea5d1e8787..3f5a1d0c30 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -409,6 +409,7 @@ qapi-schema += flat-union-inline-invalid-dict.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
-- 
2.20.0

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

> Making a discriminator conditional doesn't make much sense.  "qapi:
> add a dictionary form for TYPE" allows it, this patch fixes
> it. Eventually the two could be merged, but for clarity of review it
> is kept separate.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

I may accept the offer to let me squash it into the previous patch when
I apply.

Markus Armbruster <armbru@redhat.com> writes:

> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>
>> Making a discriminator conditional doesn't make much sense.  "qapi:

I guess it could be made to work as long as the discriminator's
condition implies each variant's condition.  Example:

    { 'union': 'BlockdevQcow2Encryption',
      'base': {
          'format': { 'type': 'BlockdevQcow2EncryptionFormat',
                      'if': 'defined(CONFIG_AES) || defined(CONFIG_LUKS)' } },
      'discriminator': 'format',
      'data': {
          'aes': { 'type': 'QCryptoBlockOptionsQCow',
                   'if': 'defined(CONFIG_AES)' },
          'luks': 'type': 'QCryptoBlockOptionsLUKS',
                  'if': 'defined(CONFIG_LUKS)' } }

Now, this example looks like a bad idea.  Whether there's a more
legitimate use remains to be seen.  Until then, outlawing conditional
discriminators is totally fine with me.

>> add a dictionary form for TYPE" allows it, this patch fixes
>> it. Eventually the two could be merged, but for clarity of review it
>> is kept separate.
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>
> I may accept the offer to let me squash it into the previous patch when
> I apply.

Squashed patch's commit message:

    qapi: Add 'if' to implicit struct members

    The generated code is for now *unconditional*.  Later patches generate
    the conditionals.

    Note that union discriminators may not have 'if' conditionals.

Avoids passing judgement :)