Changeset
tests/qapi-schema/qapi-schema-test.out             |  3 +-
18 files changed, 36 insertions(+), 168 deletions(-)
delete mode 100644 tests/qapi-schema/flat-union-incomplete-branch.json
delete mode 100644 tests/qapi-schema/flat-union-incomplete-branch.err
delete mode 100644 tests/qapi-schema/flat-union-incomplete-branch.exit
delete mode 100644 tests/qapi-schema/flat-union-incomplete-branch.out
Git apply log
Switched to a new branch '1528905228-25694-1-git-send-email-anton.nefedov@virtuozzo.com'
Applying: qapi: allow empty branches in flat unions
Applying: qapi: remove empty flat union branches and types
To https://github.com/patchew-project/qemu
 + fcf08aa...b89cfab patchew/1528905228-25694-1-git-send-email-anton.nefedov@virtuozzo.com -> patchew/1528905228-25694-1-git-send-email-anton.nefedov@virtuozzo.com (forced update)
Test passed: checkpatch

loading

Test passed: docker-mingw@fedora

loading

Test passed: s390x

loading

Test passed: docker-quick@centos7

loading

[Qemu-devel] [PATCH v4 0/2] qapi: allow empty branches in flat unions
Posted by Anton Nefedov, 1 week ago
since v3:
 - patch 2 fixed: BlockdevQcow2Encryption::QCryptoBlockOptionsQCow
   deletion was a mistake

v3: http://lists.nongnu.org/archive/html/qemu-devel/2018-05/msg06836.html

Anton Nefedov (2):
  qapi: allow empty branches in flat unions
  qapi: remove empty flat union branches and types

 docs/devel/qapi-code-gen.txt                       |  8 ++--
 qapi/block-core.json                               | 52 ++--------------------
 qapi/crypto.json                                   | 13 +-----
 qapi/misc.json                                     | 46 +------------------
 qapi/net.json                                      | 12 -----
 qapi/ui.json                                       | 19 +-------
 .../qapi-schema/flat-union-incomplete-branch.json  |  9 ----
 tests/qapi-schema/qapi-schema-test.json            |  4 +-
 block/qcow2.c                                      |  1 -
 cpus.c                                             |  2 -
 scripts/qapi/common.py                             | 11 ++---
 scripts/qapi/types.py                              |  4 +-
 scripts/qapi/visit.py                              | 17 +++++--
 tests/Makefile.include                             |  1 -
 tests/qapi-schema/flat-union-incomplete-branch.err |  1 -
 .../qapi-schema/flat-union-incomplete-branch.exit  |  1 -
 tests/qapi-schema/flat-union-incomplete-branch.out |  0
 tests/qapi-schema/qapi-schema-test.out             |  3 +-
 18 files changed, 36 insertions(+), 168 deletions(-)
 delete mode 100644 tests/qapi-schema/flat-union-incomplete-branch.json
 delete mode 100644 tests/qapi-schema/flat-union-incomplete-branch.err
 delete mode 100644 tests/qapi-schema/flat-union-incomplete-branch.exit
 delete mode 100644 tests/qapi-schema/flat-union-incomplete-branch.out

-- 
2.7.4


[Qemu-devel] [PATCH v4 1/2] qapi: allow empty branches in flat unions
Posted by Anton Nefedov, 1 week ago
It often happens that just a few discriminator values imply extra data in
a flat union. Existing checks did not make possible to leave other values
uncovered. Such cases had to be worked around by either stating a dummy
(empty) type or introducing another (subset) discriminator enumeration.

Both options create redundant entities in qapi files for little profit.

With this patch it is not necessary anymore to add designated union
fields for every possible value of a discriminator enumeration.

Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 docs/devel/qapi-code-gen.txt                        |  8 +++++---
 tests/qapi-schema/flat-union-incomplete-branch.json |  9 ---------
 tests/qapi-schema/qapi-schema-test.json             |  4 ++--
 scripts/qapi/common.py                              | 11 ++++-------
 scripts/qapi/types.py                               |  4 +++-
 scripts/qapi/visit.py                               | 17 +++++++++++++----
 tests/Makefile.include                              |  1 -
 tests/qapi-schema/flat-union-incomplete-branch.err  |  1 -
 tests/qapi-schema/flat-union-incomplete-branch.exit |  1 -
 tests/qapi-schema/flat-union-incomplete-branch.out  |  0
 tests/qapi-schema/qapi-schema-test.out              |  3 ++-
 11 files changed, 29 insertions(+), 30 deletions(-)
 delete mode 100644 tests/qapi-schema/flat-union-incomplete-branch.json
 delete mode 100644 tests/qapi-schema/flat-union-incomplete-branch.err
 delete mode 100644 tests/qapi-schema/flat-union-incomplete-branch.exit
 delete mode 100644 tests/qapi-schema/flat-union-incomplete-branch.out

diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
index 1366228..88a70e4 100644
--- a/docs/devel/qapi-code-gen.txt
+++ b/docs/devel/qapi-code-gen.txt
@@ -496,9 +496,11 @@ Resulting in these JSON objects:
 
 Notice that in a flat union, the discriminator name is controlled by
 the user, but because it must map to a base member with enum type, the
-code generator can ensure that branches exist for all values of the
-enum (although the order of the keys need not match the declaration of
-the enum).  In the resulting generated C data types, a flat union is
+code generator ensures that branches match the existing values of the
+enum. The order of the keys need not match the declaration of the enum.
+The keys need not cover all possible enum values. Omitted enum values
+are still valid branches that add no additional members to the data type.
+In the resulting generated C data types, a flat union is
 represented as a struct with the base members included directly, and
 then a union of structures for each branch of the struct.
 
diff --git a/tests/qapi-schema/flat-union-incomplete-branch.json b/tests/qapi-schema/flat-union-incomplete-branch.json
deleted file mode 100644
index 25a411b..0000000
--- a/tests/qapi-schema/flat-union-incomplete-branch.json
+++ /dev/null
@@ -1,9 +0,0 @@
-# we require all branches of the union to be covered
-{ 'enum': 'TestEnum',
-  'data': [ 'value1', 'value2' ] }
-{ 'struct': 'TestTypeA',
-  'data': { 'string': 'str' } }
-{ 'union': 'TestUnion',
-  'base': { 'type': 'TestEnum' },
-  'discriminator': 'type',
-  'data': { 'value1': 'TestTypeA' } }
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 46c7282..881b53b 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -39,7 +39,7 @@
             '*enum1': 'EnumOne' } }   # intentional forward reference
 
 { 'enum': 'EnumOne',
-  'data': [ 'value1', 'value2', 'value3' ] }
+  'data': [ 'value1', 'value2', 'value3', 'value4' ] }
 
 { 'struct': 'UserDefZero',
   'data': { 'integer': 'int' } }
@@ -76,7 +76,7 @@
   'discriminator': 'enum1',
   'data': { 'value1' : 'UserDefA',
             'value2' : 'UserDefB',
-            'value3' : 'UserDefB' } }
+            'value3' : 'UserDefB' } } # skip 'value4'
 
 { 'struct': 'UserDefUnionBase',
   'base': 'UserDefZero',
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index 2462fc0..0f3d89b 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -779,13 +779,6 @@ def check_union(expr, info):
                                    "enum '%s'"
                                    % (key, enum_define['enum']))
 
-    # If discriminator is user-defined, ensure all values are covered
-    if enum_define:
-        for value in enum_define['data']:
-            if value not in members.keys():
-                raise QAPISemError(info, "Union '%s' data missing '%s' branch"
-                                   % (name, value))
-
 
 def check_alternate(expr, info):
     name = expr['alternate']
@@ -1644,6 +1637,10 @@ class QAPISchema(object):
         if tag_name:
             variants = [self._make_variant(key, value)
                         for (key, value) in data.items()]
+            # branches that are not explicitly covered get an empty type
+            variants += [self._make_variant(key, 'q_empty')
+                         for key in discriminator_find_enum_define(expr)['data']
+                         if key not in data.keys()]
             members = []
         else:
             variants = [self._make_simple_variant(key, value, info)
diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
index 64d9c0f..1fb2b6d 100644
--- a/scripts/qapi/types.py
+++ b/scripts/qapi/types.py
@@ -124,7 +124,9 @@ def gen_variants(variants):
 ''',
                 c_name=c_name(variants.tag_member.name))
 
-    for var in variants.variants:
+    # filter out the empty types
+    for var in filter(lambda var: var.type.name != 'q_empty',
+                      variants.variants):
         ret += mcgen('''
         %(c_type)s %(c_name)s;
 ''',
diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
index 5d72d89..96bd32c 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -81,14 +81,23 @@ void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp)
                      c_name=c_name(variants.tag_member.name))
 
         for var in variants.variants:
-            ret += mcgen('''
+            case_str = c_enum_const(variants.tag_member.type.name,
+                                    var.name,
+                                    variants.tag_member.type.prefix)
+            if var.type.name == 'q_empty':
+                # valid variant and nothing to do
+                ret += mcgen('''
+    case %(case)s:
+        break;
+''',
+                             case=case_str)
+            else:
+                ret += mcgen('''
     case %(case)s:
         visit_type_%(c_type)s_members(v, &obj->u.%(c_name)s, &err);
         break;
 ''',
-                         case=c_enum_const(variants.tag_member.type.name,
-                                           var.name,
-                                           variants.tag_member.type.prefix),
+                         case=case_str,
                          c_type=var.type.c_name(), c_name=c_name(var.name))
 
         ret += mcgen('''
diff --git a/tests/Makefile.include b/tests/Makefile.include
index bb08e37..afdd0db 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -496,7 +496,6 @@ qapi-schema += flat-union-base-any.json
 qapi-schema += flat-union-base-union.json
 qapi-schema += flat-union-clash-member.json
 qapi-schema += flat-union-empty.json
-qapi-schema += flat-union-incomplete-branch.json
 qapi-schema += flat-union-inline.json
 qapi-schema += flat-union-int-branch.json
 qapi-schema += flat-union-invalid-branch-key.json
diff --git a/tests/qapi-schema/flat-union-incomplete-branch.err b/tests/qapi-schema/flat-union-incomplete-branch.err
deleted file mode 100644
index e826bf0..0000000
--- a/tests/qapi-schema/flat-union-incomplete-branch.err
+++ /dev/null
@@ -1 +0,0 @@
-tests/qapi-schema/flat-union-incomplete-branch.json:6: Union 'TestUnion' data missing 'value2' branch
diff --git a/tests/qapi-schema/flat-union-incomplete-branch.exit b/tests/qapi-schema/flat-union-incomplete-branch.exit
deleted file mode 100644
index d00491f..0000000
--- a/tests/qapi-schema/flat-union-incomplete-branch.exit
+++ /dev/null
@@ -1 +0,0 @@
-1
diff --git a/tests/qapi-schema/flat-union-incomplete-branch.out b/tests/qapi-schema/flat-union-incomplete-branch.out
deleted file mode 100644
index e69de29..0000000
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 542a19c..0dbcdaf 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -23,7 +23,7 @@ object UserDefOne
     base UserDefZero
     member string: str optional=False
     member enum1: EnumOne optional=True
-enum EnumOne ['value1', 'value2', 'value3']
+enum EnumOne ['value1', 'value2', 'value3', 'value4']
 object UserDefZero
     member integer: int optional=False
 object UserDefTwoDictDict
@@ -52,6 +52,7 @@ object UserDefFlatUnion
     case value1: UserDefA
     case value2: UserDefB
     case value3: UserDefB
+    case value4: q_empty
 object UserDefUnionBase
     base UserDefZero
     member string: str optional=False
-- 
2.7.4


Re: [Qemu-devel] [PATCH v4 1/2] qapi: allow empty branches in flat unions
Posted by Markus Armbruster, 1 week ago
Anton Nefedov <anton.nefedov@virtuozzo.com> writes:

> It often happens that just a few discriminator values imply extra data in
> a flat union. Existing checks did not make possible to leave other values
> uncovered. Such cases had to be worked around by either stating a dummy
> (empty) type or introducing another (subset) discriminator enumeration.
>
> Both options create redundant entities in qapi files for little profit.
>
> With this patch it is not necessary anymore to add designated union
> fields for every possible value of a discriminator enumeration.
>
> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  docs/devel/qapi-code-gen.txt                        |  8 +++++---
>  tests/qapi-schema/flat-union-incomplete-branch.json |  9 ---------
>  tests/qapi-schema/qapi-schema-test.json             |  4 ++--
>  scripts/qapi/common.py                              | 11 ++++-------
>  scripts/qapi/types.py                               |  4 +++-
>  scripts/qapi/visit.py                               | 17 +++++++++++++----
>  tests/Makefile.include                              |  1 -
>  tests/qapi-schema/flat-union-incomplete-branch.err  |  1 -
>  tests/qapi-schema/flat-union-incomplete-branch.exit |  1 -
>  tests/qapi-schema/flat-union-incomplete-branch.out  |  0
>  tests/qapi-schema/qapi-schema-test.out              |  3 ++-
>  11 files changed, 29 insertions(+), 30 deletions(-)
>  delete mode 100644 tests/qapi-schema/flat-union-incomplete-branch.json
>  delete mode 100644 tests/qapi-schema/flat-union-incomplete-branch.err
>  delete mode 100644 tests/qapi-schema/flat-union-incomplete-branch.exit
>  delete mode 100644 tests/qapi-schema/flat-union-incomplete-branch.out
>
> diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
> index 1366228..88a70e4 100644
> --- a/docs/devel/qapi-code-gen.txt
> +++ b/docs/devel/qapi-code-gen.txt
> @@ -496,9 +496,11 @@ Resulting in these JSON objects:
>  
>  Notice that in a flat union, the discriminator name is controlled by
>  the user, but because it must map to a base member with enum type, the
> -code generator can ensure that branches exist for all values of the
> -enum (although the order of the keys need not match the declaration of
> -the enum).  In the resulting generated C data types, a flat union is
> +code generator ensures that branches match the existing values of the
> +enum. The order of the keys need not match the declaration of the enum.
> +The keys need not cover all possible enum values. Omitted enum values
> +are still valid branches that add no additional members to the data type.
> +In the resulting generated C data types, a flat union is
>  represented as a struct with the base members included directly, and
>  then a union of structures for each branch of the struct.
>  
> diff --git a/tests/qapi-schema/flat-union-incomplete-branch.json b/tests/qapi-schema/flat-union-incomplete-branch.json
> deleted file mode 100644
> index 25a411b..0000000
> --- a/tests/qapi-schema/flat-union-incomplete-branch.json
> +++ /dev/null
> @@ -1,9 +0,0 @@
> -# we require all branches of the union to be covered
> -{ 'enum': 'TestEnum',
> -  'data': [ 'value1', 'value2' ] }
> -{ 'struct': 'TestTypeA',
> -  'data': { 'string': 'str' } }
> -{ 'union': 'TestUnion',
> -  'base': { 'type': 'TestEnum' },
> -  'discriminator': 'type',
> -  'data': { 'value1': 'TestTypeA' } }
> diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
> index 46c7282..881b53b 100644
> --- a/tests/qapi-schema/qapi-schema-test.json
> +++ b/tests/qapi-schema/qapi-schema-test.json
> @@ -39,7 +39,7 @@
>              '*enum1': 'EnumOne' } }   # intentional forward reference
>  
>  { 'enum': 'EnumOne',
> -  'data': [ 'value1', 'value2', 'value3' ] }
> +  'data': [ 'value1', 'value2', 'value3', 'value4' ] }
>  
>  { 'struct': 'UserDefZero',
>    'data': { 'integer': 'int' } }
> @@ -76,7 +76,7 @@
>    'discriminator': 'enum1',
>    'data': { 'value1' : 'UserDefA',
>              'value2' : 'UserDefB',
> -            'value3' : 'UserDefB' } }
> +            'value3' : 'UserDefB' } } # skip 'value4'

Perhaps

               'value3' : 'UserDefB'
                # 'value4' defaults to empty
             } }

would be clearer.

>  
>  { 'struct': 'UserDefUnionBase',
>    'base': 'UserDefZero',
> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> index 2462fc0..0f3d89b 100644
> --- a/scripts/qapi/common.py
> +++ b/scripts/qapi/common.py
> @@ -779,13 +779,6 @@ def check_union(expr, info):
>                                     "enum '%s'"
>                                     % (key, enum_define['enum']))
>  
> -    # If discriminator is user-defined, ensure all values are covered
> -    if enum_define:
> -        for value in enum_define['data']:
> -            if value not in members.keys():
> -                raise QAPISemError(info, "Union '%s' data missing '%s' branch"
> -                                   % (name, value))
> -
>  
>  def check_alternate(expr, info):
>      name = expr['alternate']
> @@ -1644,6 +1637,10 @@ class QAPISchema(object):
>          if tag_name:
>              variants = [self._make_variant(key, value)
>                          for (key, value) in data.items()]
> +            # branches that are not explicitly covered get an empty type
> +            variants += [self._make_variant(key, 'q_empty')
> +                         for key in discriminator_find_enum_define(expr)['data']

Long line, flagged by pycodestyle --diff.

> +                         if key not in data.keys()]
>              members = []
>          else:
>              variants = [self._make_simple_variant(key, value, info)

The way you desugar the union is simple enough, but it uses
discriminator_find_enum_define() outside check_exprs(), which I'd rather
avoid.  Not something you could be reasonably expected to know.

We could desugar in QAPISchemaObjectTypeVariants.check(), where we have
self.tag_member.type.values.

   @@ -1346,10 +1346,17 @@ class QAPISchemaObjectTypeVariants(object):
                v.set_owner(name)

        def check(self, schema, seen):
   -        if not self.tag_member:    # flat union
   +        if not self.tag_member: # flat union
                self.tag_member = seen[c_name(self._tag_name)]
                assert self._tag_name == self.tag_member.name
            assert isinstance(self.tag_member.type, QAPISchemaEnumType)
   +        if self._tag_name:      # flat union
   +            cases = set([v.name for v in self.variants])
   +            for val in self.tag_member.type.values:
   +                if val.name not in cases:
   +                    v = QAPISchemaObjectTypeVariant(val.name, 'q_empty')
   +                    v.set_owner(self.tag_member.owner)
   +                    self.variants.append(v)
            for v in self.variants:
                v.check(schema)
                # Union names must match enum values; alternate names are

Not so simple anymore.

Simpler: desugar in check_union(), i.e. replace your two hunks by just

   @@ -783,8 +783,7 @@ def check_union(expr, info):
        if enum_define:
            for value in enum_define['data']:
                if value not in members.keys():
   -                raise QAPISemError(info, "Union '%s' data missing '%s' branch"
   -                                   % (name, value))
   +                members[value] = 'q_empty'


    def check_alternate(expr, info):

What do you think?

> diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
> index 64d9c0f..1fb2b6d 100644
> --- a/scripts/qapi/types.py
> +++ b/scripts/qapi/types.py
> @@ -124,7 +124,9 @@ def gen_variants(variants):
>  ''',
>                  c_name=c_name(variants.tag_member.name))
>  
> -    for var in variants.variants:
> +    # filter out the empty types
> +    for var in filter(lambda var: var.type.name != 'q_empty',
> +                      variants.variants):

The comment's position suggests it tells what the loop does, but that's
not the case.  Let's do it the old-fashioned obvious way instead:

       for var in variants.variants:
           if var.type.name == 'q_empty':
               continue

Aside: we need to filter out q_empty, because we don't generate a C type
for it.  Perhaps generating one would be simpler, but that's beyond the
scope of this patch.

>          ret += mcgen('''
>          %(c_type)s %(c_name)s;
>  ''',
> diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
> index 5d72d89..96bd32c 100644
> --- a/scripts/qapi/visit.py
> +++ b/scripts/qapi/visit.py
> @@ -81,14 +81,23 @@ void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp)
>                       c_name=c_name(variants.tag_member.name))
>  
>          for var in variants.variants:
> -            ret += mcgen('''
> +            case_str = c_enum_const(variants.tag_member.type.name,
> +                                    var.name,
> +                                    variants.tag_member.type.prefix)
> +            if var.type.name == 'q_empty':
> +                # valid variant and nothing to do
> +                ret += mcgen('''
> +    case %(case)s:
> +        break;
> +''',
> +                             case=case_str)
> +            else:
> +                ret += mcgen('''

Aside: we need this, because we don't generate a visitor for q_empty.

>      case %(case)s:
>          visit_type_%(c_type)s_members(v, &obj->u.%(c_name)s, &err);
>          break;
>  ''',
> -                         case=c_enum_const(variants.tag_member.type.name,
> -                                           var.name,
> -                                           variants.tag_member.type.prefix),
> +                         case=case_str,
>                           c_type=var.type.c_name(), c_name=c_name(var.name))

Indentation's off, flagged by pycodestyle --diff.

>  
>          ret += mcgen('''
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index bb08e37..afdd0db 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -496,7 +496,6 @@ qapi-schema += flat-union-base-any.json
>  qapi-schema += flat-union-base-union.json
>  qapi-schema += flat-union-clash-member.json
>  qapi-schema += flat-union-empty.json
> -qapi-schema += flat-union-incomplete-branch.json
>  qapi-schema += flat-union-inline.json
>  qapi-schema += flat-union-int-branch.json
>  qapi-schema += flat-union-invalid-branch-key.json
> diff --git a/tests/qapi-schema/flat-union-incomplete-branch.err b/tests/qapi-schema/flat-union-incomplete-branch.err
> deleted file mode 100644
> index e826bf0..0000000
> --- a/tests/qapi-schema/flat-union-incomplete-branch.err
> +++ /dev/null
> @@ -1 +0,0 @@
> -tests/qapi-schema/flat-union-incomplete-branch.json:6: Union 'TestUnion' data missing 'value2' branch
> diff --git a/tests/qapi-schema/flat-union-incomplete-branch.exit b/tests/qapi-schema/flat-union-incomplete-branch.exit
> deleted file mode 100644
> index d00491f..0000000
> --- a/tests/qapi-schema/flat-union-incomplete-branch.exit
> +++ /dev/null
> @@ -1 +0,0 @@
> -1
> diff --git a/tests/qapi-schema/flat-union-incomplete-branch.out b/tests/qapi-schema/flat-union-incomplete-branch.out
> deleted file mode 100644
> index e69de29..0000000
> diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
> index 542a19c..0dbcdaf 100644
> --- a/tests/qapi-schema/qapi-schema-test.out
> +++ b/tests/qapi-schema/qapi-schema-test.out
> @@ -23,7 +23,7 @@ object UserDefOne
>      base UserDefZero
>      member string: str optional=False
>      member enum1: EnumOne optional=True
> -enum EnumOne ['value1', 'value2', 'value3']
> +enum EnumOne ['value1', 'value2', 'value3', 'value4']
>  object UserDefZero
>      member integer: int optional=False
>  object UserDefTwoDictDict
> @@ -52,6 +52,7 @@ object UserDefFlatUnion
>      case value1: UserDefA
>      case value2: UserDefB
>      case value3: UserDefB
> +    case value4: q_empty
>  object UserDefUnionBase
>      base UserDefZero
>      member string: str optional=False

Re: [Qemu-devel] [PATCH v4 1/2] qapi: allow empty branches in flat unions
Posted by Anton Nefedov, 1 week ago

On 14/6/2018 10:19 AM, Markus Armbruster wrote:
> Anton Nefedov <anton.nefedov@virtuozzo.com> writes:
> 
>> It often happens that just a few discriminator values imply extra data in
>> a flat union. Existing checks did not make possible to leave other values
>> uncovered. Such cases had to be worked around by either stating a dummy
>> (empty) type or introducing another (subset) discriminator enumeration.
>>
>> Both options create redundant entities in qapi files for little profit.
>>
>> With this patch it is not necessary anymore to add designated union
>> fields for every possible value of a discriminator enumeration.
>>
>> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> ---
>>   docs/devel/qapi-code-gen.txt                        |  8 +++++---
>>   tests/qapi-schema/flat-union-incomplete-branch.json |  9 ---------
>>   tests/qapi-schema/qapi-schema-test.json             |  4 ++--
>>   scripts/qapi/common.py                              | 11 ++++-------
>>   scripts/qapi/types.py                               |  4 +++-
>>   scripts/qapi/visit.py                               | 17 +++++++++++++----
>>   tests/Makefile.include                              |  1 -
>>   tests/qapi-schema/flat-union-incomplete-branch.err  |  1 -
>>   tests/qapi-schema/flat-union-incomplete-branch.exit |  1 -
>>   tests/qapi-schema/flat-union-incomplete-branch.out  |  0
>>   tests/qapi-schema/qapi-schema-test.out              |  3 ++-
>>   11 files changed, 29 insertions(+), 30 deletions(-)
>>   delete mode 100644 tests/qapi-schema/flat-union-incomplete-branch.json
>>   delete mode 100644 tests/qapi-schema/flat-union-incomplete-branch.err
>>   delete mode 100644 tests/qapi-schema/flat-union-incomplete-branch.exit
>>   delete mode 100644 tests/qapi-schema/flat-union-incomplete-branch.out
>>
>> diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
>> index 1366228..88a70e4 100644
>> --- a/docs/devel/qapi-code-gen.txt
>> +++ b/docs/devel/qapi-code-gen.txt
>> @@ -496,9 +496,11 @@ Resulting in these JSON objects:
>>   
>>   Notice that in a flat union, the discriminator name is controlled by
>>   the user, but because it must map to a base member with enum type, the
>> -code generator can ensure that branches exist for all values of the
>> -enum (although the order of the keys need not match the declaration of
>> -the enum).  In the resulting generated C data types, a flat union is
>> +code generator ensures that branches match the existing values of the
>> +enum. The order of the keys need not match the declaration of the enum.
>> +The keys need not cover all possible enum values. Omitted enum values
>> +are still valid branches that add no additional members to the data type.
>> +In the resulting generated C data types, a flat union is
>>   represented as a struct with the base members included directly, and
>>   then a union of structures for each branch of the struct.
>>   
>> diff --git a/tests/qapi-schema/flat-union-incomplete-branch.json b/tests/qapi-schema/flat-union-incomplete-branch.json
>> deleted file mode 100644
>> index 25a411b..0000000
>> --- a/tests/qapi-schema/flat-union-incomplete-branch.json
>> +++ /dev/null
>> @@ -1,9 +0,0 @@
>> -# we require all branches of the union to be covered
>> -{ 'enum': 'TestEnum',
>> -  'data': [ 'value1', 'value2' ] }
>> -{ 'struct': 'TestTypeA',
>> -  'data': { 'string': 'str' } }
>> -{ 'union': 'TestUnion',
>> -  'base': { 'type': 'TestEnum' },
>> -  'discriminator': 'type',
>> -  'data': { 'value1': 'TestTypeA' } }
>> diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
>> index 46c7282..881b53b 100644
>> --- a/tests/qapi-schema/qapi-schema-test.json
>> +++ b/tests/qapi-schema/qapi-schema-test.json
>> @@ -39,7 +39,7 @@
>>               '*enum1': 'EnumOne' } }   # intentional forward reference
>>   
>>   { 'enum': 'EnumOne',
>> -  'data': [ 'value1', 'value2', 'value3' ] }
>> +  'data': [ 'value1', 'value2', 'value3', 'value4' ] }
>>   
>>   { 'struct': 'UserDefZero',
>>     'data': { 'integer': 'int' } }
>> @@ -76,7 +76,7 @@
>>     'discriminator': 'enum1',
>>     'data': { 'value1' : 'UserDefA',
>>               'value2' : 'UserDefB',
>> -            'value3' : 'UserDefB' } }
>> +            'value3' : 'UserDefB' } } # skip 'value4'
> 
> Perhaps
> 
>                 'value3' : 'UserDefB'
>                  # 'value4' defaults to empty
>               } }
> 
> would be clearer.
> 

Sure

>>   
>>   { 'struct': 'UserDefUnionBase',
>>     'base': 'UserDefZero',
>> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
>> index 2462fc0..0f3d89b 100644
>> --- a/scripts/qapi/common.py
>> +++ b/scripts/qapi/common.py
>> @@ -779,13 +779,6 @@ def check_union(expr, info):
>>                                      "enum '%s'"
>>                                      % (key, enum_define['enum']))
>>   
>> -    # If discriminator is user-defined, ensure all values are covered
>> -    if enum_define:
>> -        for value in enum_define['data']:
>> -            if value not in members.keys():
>> -                raise QAPISemError(info, "Union '%s' data missing '%s' branch"
>> -                                   % (name, value))
>> -
>>   
>>   def check_alternate(expr, info):
>>       name = expr['alternate']
>> @@ -1644,6 +1637,10 @@ class QAPISchema(object):
>>           if tag_name:
>>               variants = [self._make_variant(key, value)
>>                           for (key, value) in data.items()]
>> +            # branches that are not explicitly covered get an empty type
>> +            variants += [self._make_variant(key, 'q_empty')
>> +                         for key in discriminator_find_enum_define(expr)['data']
> 
> Long line, flagged by pycodestyle --diff.
> 

Done.

>> +                         if key not in data.keys()]
>>               members = []
>>           else:
>>               variants = [self._make_simple_variant(key, value, info)
> 
> The way you desugar the union is simple enough, but it uses
> discriminator_find_enum_define() outside check_exprs(), which I'd rather
> avoid.  Not something you could be reasonably expected to know.
> 
> We could desugar in QAPISchemaObjectTypeVariants.check(), where we have
> self.tag_member.type.values.
> 
>     @@ -1346,10 +1346,17 @@ class QAPISchemaObjectTypeVariants(object):
>                  v.set_owner(name)
> 
>          def check(self, schema, seen):
>     -        if not self.tag_member:    # flat union
>     +        if not self.tag_member: # flat union
>                  self.tag_member = seen[c_name(self._tag_name)]
>                  assert self._tag_name == self.tag_member.name
>              assert isinstance(self.tag_member.type, QAPISchemaEnumType)
>     +        if self._tag_name:      # flat union
>     +            cases = set([v.name for v in self.variants])
>     +            for val in self.tag_member.type.values:
>     +                if val.name not in cases:
>     +                    v = QAPISchemaObjectTypeVariant(val.name, 'q_empty')
>     +                    v.set_owner(self.tag_member.owner)
>     +                    self.variants.append(v)
>              for v in self.variants:
>                  v.check(schema)
>                  # Union names must match enum values; alternate names are
> 
> Not so simple anymore.
> 
> Simpler: desugar in check_union(), i.e. replace your two hunks by just
> 
>     @@ -783,8 +783,7 @@ def check_union(expr, info):
>          if enum_define:
>              for value in enum_define['data']:
>                  if value not in members.keys():
>     -                raise QAPISemError(info, "Union '%s' data missing '%s' branch"
>     -                                   % (name, value))
>     +                members[value] = 'q_empty'
> 
> 
>      def check_alternate(expr, info):
> 
> What do you think?
> 

it didn't feel right aesthetically to normalize by modifying the parsed
expressions especially at check_exprs() stage. But maybe using
discriminator_find_enum_define() is more sloppy, I can't quite see

>> diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
>> index 64d9c0f..1fb2b6d 100644
>> --- a/scripts/qapi/types.py
>> +++ b/scripts/qapi/types.py
>> @@ -124,7 +124,9 @@ def gen_variants(variants):
>>   ''',
>>                   c_name=c_name(variants.tag_member.name))
>>   
>> -    for var in variants.variants:
>> +    # filter out the empty types
>> +    for var in filter(lambda var: var.type.name != 'q_empty',
>> +                      variants.variants):
> 
> The comment's position suggests it tells what the loop does, but that's
> not the case.  Let's do it the old-fashioned obvious way instead:
> 
>         for var in variants.variants:
>             if var.type.name == 'q_empty':
>                 continue
> 

Done.

> Aside: we need to filter out q_empty, because we don't generate a C type
> for it.  Perhaps generating one would be simpler, but that's beyond the
> scope of this patch.
> 
>>           ret += mcgen('''
>>           %(c_type)s %(c_name)s;
>>   ''',
>> diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
>> index 5d72d89..96bd32c 100644
>> --- a/scripts/qapi/visit.py
>> +++ b/scripts/qapi/visit.py
>> @@ -81,14 +81,23 @@ void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp)
>>                        c_name=c_name(variants.tag_member.name))
>>   
>>           for var in variants.variants:
>> -            ret += mcgen('''
>> +            case_str = c_enum_const(variants.tag_member.type.name,
>> +                                    var.name,
>> +                                    variants.tag_member.type.prefix)
>> +            if var.type.name == 'q_empty':
>> +                # valid variant and nothing to do
>> +                ret += mcgen('''
>> +    case %(case)s:
>> +        break;
>> +''',
>> +                             case=case_str)
>> +            else:
>> +                ret += mcgen('''
> 
> Aside: we need this, because we don't generate a visitor for q_empty.
> 
>>       case %(case)s:
>>           visit_type_%(c_type)s_members(v, &obj->u.%(c_name)s, &err);
>>           break;
>>   ''',
>> -                         case=c_enum_const(variants.tag_member.type.name,
>> -                                           var.name,
>> -                                           variants.tag_member.type.prefix),
>> +                         case=case_str,
>>                            c_type=var.type.c_name(), c_name=c_name(var.name))
> 
> Indentation's off, flagged by pycodestyle --diff.
> 

Done.

>>   
>>           ret += mcgen('''
>> diff --git a/tests/Makefile.include b/tests/Makefile.include
>> index bb08e37..afdd0db 100644
>> --- a/tests/Makefile.include
>> +++ b/tests/Makefile.include
>> @@ -496,7 +496,6 @@ qapi-schema += flat-union-base-any.json
>>   qapi-schema += flat-union-base-union.json
>>   qapi-schema += flat-union-clash-member.json
>>   qapi-schema += flat-union-empty.json
>> -qapi-schema += flat-union-incomplete-branch.json
>>   qapi-schema += flat-union-inline.json
>>   qapi-schema += flat-union-int-branch.json
>>   qapi-schema += flat-union-invalid-branch-key.json
>> diff --git a/tests/qapi-schema/flat-union-incomplete-branch.err b/tests/qapi-schema/flat-union-incomplete-branch.err
>> deleted file mode 100644
>> index e826bf0..0000000
>> --- a/tests/qapi-schema/flat-union-incomplete-branch.err
>> +++ /dev/null
>> @@ -1 +0,0 @@
>> -tests/qapi-schema/flat-union-incomplete-branch.json:6: Union 'TestUnion' data missing 'value2' branch
>> diff --git a/tests/qapi-schema/flat-union-incomplete-branch.exit b/tests/qapi-schema/flat-union-incomplete-branch.exit
>> deleted file mode 100644
>> index d00491f..0000000
>> --- a/tests/qapi-schema/flat-union-incomplete-branch.exit
>> +++ /dev/null
>> @@ -1 +0,0 @@
>> -1
>> diff --git a/tests/qapi-schema/flat-union-incomplete-branch.out b/tests/qapi-schema/flat-union-incomplete-branch.out
>> deleted file mode 100644
>> index e69de29..0000000
>> diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
>> index 542a19c..0dbcdaf 100644
>> --- a/tests/qapi-schema/qapi-schema-test.out
>> +++ b/tests/qapi-schema/qapi-schema-test.out
>> @@ -23,7 +23,7 @@ object UserDefOne
>>       base UserDefZero
>>       member string: str optional=False
>>       member enum1: EnumOne optional=True
>> -enum EnumOne ['value1', 'value2', 'value3']
>> +enum EnumOne ['value1', 'value2', 'value3', 'value4']
>>   object UserDefZero
>>       member integer: int optional=False
>>   object UserDefTwoDictDict
>> @@ -52,6 +52,7 @@ object UserDefFlatUnion
>>       case value1: UserDefA
>>       case value2: UserDefB
>>       case value3: UserDefB
>> +    case value4: q_empty
>>   object UserDefUnionBase
>>       base UserDefZero
>>       member string: str optional=False

Re: [Qemu-devel] [PATCH v4 1/2] qapi: allow empty branches in flat unions
Posted by Markus Armbruster, 1 week ago
Anton Nefedov <anton.nefedov@virtuozzo.com> writes:

> On 14/6/2018 10:19 AM, Markus Armbruster wrote:
>> Anton Nefedov <anton.nefedov@virtuozzo.com> writes:
>>
>>> It often happens that just a few discriminator values imply extra data in
>>> a flat union. Existing checks did not make possible to leave other values
>>> uncovered. Such cases had to be worked around by either stating a dummy
>>> (empty) type or introducing another (subset) discriminator enumeration.
>>>
>>> Both options create redundant entities in qapi files for little profit.
>>>
>>> With this patch it is not necessary anymore to add designated union
>>> fields for every possible value of a discriminator enumeration.
>>>
>>> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
>>> Reviewed-by: Eric Blake <eblake@redhat.com>
[...]
>>> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
>>> index 2462fc0..0f3d89b 100644
>>> --- a/scripts/qapi/common.py
>>> +++ b/scripts/qapi/common.py
>>> @@ -779,13 +779,6 @@ def check_union(expr, info):
>>>                                      "enum '%s'"
>>>                                      % (key, enum_define['enum']))
>>>   -    # If discriminator is user-defined, ensure all values are
>>> covered
>>> -    if enum_define:
>>> -        for value in enum_define['data']:
>>> -            if value not in members.keys():
>>> -                raise QAPISemError(info, "Union '%s' data missing '%s' branch"
>>> -                                   % (name, value))
>>> -
>>>     def check_alternate(expr, info):
>>>       name = expr['alternate']
>>> @@ -1644,6 +1637,10 @@ class QAPISchema(object):
>>>           if tag_name:
>>>               variants = [self._make_variant(key, value)
>>>                           for (key, value) in data.items()]
>>> +            # branches that are not explicitly covered get an empty type
>>> +            variants += [self._make_variant(key, 'q_empty')
>>> +                         for key in discriminator_find_enum_define(expr)['data']
>>
>> Long line, flagged by pycodestyle --diff.
>>
>
> Done.
>
>>> +                         if key not in data.keys()]
>>>               members = []
>>>           else:
>>>               variants = [self._make_simple_variant(key, value, info)
>>
>> The way you desugar the union is simple enough, but it uses
>> discriminator_find_enum_define() outside check_exprs(), which I'd rather
>> avoid.  Not something you could be reasonably expected to know.
>>
>> We could desugar in QAPISchemaObjectTypeVariants.check(), where we have
>> self.tag_member.type.values.
>>
>>     @@ -1346,10 +1346,17 @@ class QAPISchemaObjectTypeVariants(object):
>>                  v.set_owner(name)
>>
>>          def check(self, schema, seen):
>>     -        if not self.tag_member:    # flat union
>>     +        if not self.tag_member: # flat union
>>                  self.tag_member = seen[c_name(self._tag_name)]
>>                  assert self._tag_name == self.tag_member.name
>>              assert isinstance(self.tag_member.type, QAPISchemaEnumType)
>>     +        if self._tag_name:      # flat union
>>     +            cases = set([v.name for v in self.variants])
>>     +            for val in self.tag_member.type.values:
>>     +                if val.name not in cases:
>>     +                    v = QAPISchemaObjectTypeVariant(val.name, 'q_empty')
>>     +                    v.set_owner(self.tag_member.owner)
>>     +                    self.variants.append(v)
>>              for v in self.variants:
>>                  v.check(schema)
>>                  # Union names must match enum values; alternate names are
>>
>> Not so simple anymore.
>>
>> Simpler: desugar in check_union(), i.e. replace your two hunks by just
>>
>>     @@ -783,8 +783,7 @@ def check_union(expr, info):
>>          if enum_define:
>>              for value in enum_define['data']:
>>                  if value not in members.keys():
>>     -                raise QAPISemError(info, "Union '%s' data missing '%s' branch"
>>     -                                   % (name, value))
>>     +                members[value] = 'q_empty'
>>
>>
>>      def check_alternate(expr, info):
>>
>> What do you think?
>>
>
> it didn't feel right aesthetically to normalize by modifying the parsed
> expressions especially at check_exprs() stage. But maybe using
> discriminator_find_enum_define() is more sloppy, I can't quite see

The issue isn't sloppiness, it's adding to our technical debt.  To
explain this, I need to relate a bit of history.

In the beginning, we did everything with syntax trees.  Commit
ac88219a6c7 (Sep 2015) spliced in an intermediate representation.  Its
commit message explains:

    The QAPI code generators work with a syntax tree (nested dictionaries)
    plus a few symbol tables (also dictionaries) on the side.

    They have clearly outgrown these simple data structures.  There's lots
    of rummaging around in dictionaries, and information is recomputed on
    the fly.  For the work I'm going to do, I want more clearly defined
    and more convenient interfaces.
    
    Going forward, I also want less coupling between the back-ends and the
    syntax tree, to make messing with the syntax easier.

We accepted some technical debt back then.  Same commit message:

    Shortcut: the semantic analysis still relies on existing check_exprs()
    to do the actual semantic checking.  All this code needs to move into
    the classes.  Mark as TODO.

It's this one:

    #
    # Semantic analysis of schema expressions
    # TODO fold into QAPISchema
    # TODO catching name collisions in generated code would be nice
    #

check_exprs() is the only entry point into the code under this TODO.
Your patch adds another one.  I don't like that.

Nobody expects you to deduce this from the code by yourself :)

Now, I agree hacking up the syntax tree isn't particularly clean,
either.  Perhaps doing it in QAPISchemaObjectTypeVariants.check() is
best after all.  In case you feel unsure about making that change, I can
take your solution, and address its technical debt issue in a follow-up
patch.

[...]

[Qemu-devel] [PATCH v4 2/2] qapi: remove empty flat union branches and types
Posted by Anton Nefedov, 1 week ago
Flat unions may now have uncovered branches, so it is possible to get rid
of empty types defined for that purpose only.

Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
---
 qapi/block-core.json | 52 +++-------------------------------------------------
 qapi/crypto.json     | 13 +------------
 qapi/misc.json       | 46 ++--------------------------------------------
 qapi/net.json        | 12 ------------
 qapi/ui.json         | 19 +------------------
 block/qcow2.c        |  1 -
 cpus.c               |  2 --
 7 files changed, 7 insertions(+), 138 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index fff23fc..b3e94d8 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -52,8 +52,7 @@
 { 'union': 'ImageInfoSpecificQCow2Encryption',
   'base': 'ImageInfoSpecificQCow2EncryptionBase',
   'discriminator': 'format',
-  'data': { 'aes': 'QCryptoBlockInfoQCow',
-            'luks': 'QCryptoBlockInfoLUKS' } }
+  'data': { 'luks': 'QCryptoBlockInfoLUKS' } }
 
 ##
 # @ImageInfoSpecificQCow2:
@@ -2853,16 +2852,6 @@
             'hash': 'str' }}
 
 ##
-# @SshHostKeyDummy:
-#
-# For those union branches that don't need additional fields.
-#
-# Since: 2.12
-##
-{ 'struct': 'SshHostKeyDummy',
-  'data': {} }
-
-##
 # @SshHostKeyCheck:
 #
 # Since: 2.12
@@ -2870,9 +2859,7 @@
 { 'union': 'SshHostKeyCheck',
   'base': { 'mode': 'SshHostKeyCheckMode' },
   'discriminator': 'mode',
-  'data': { 'none': 'SshHostKeyDummy',
-            'hash': 'SshHostKeyHash',
-            'known_hosts': 'SshHostKeyDummy' } }
+  'data': { 'hash': 'SshHostKeyHash' } }
 
 ##
 # @BlockdevOptionsSsh:
@@ -4032,15 +4019,6 @@
             '*force-size':          'bool' } }
 
 ##
-# @BlockdevCreateNotSupported:
-#
-# This is used for all drivers that don't support creating images.
-#
-# Since: 2.12
-##
-{ 'struct': 'BlockdevCreateNotSupported', 'data': {}}
-
-##
 # @BlockdevCreateOptions:
 #
 # Options for creating an image format on a given node.
@@ -4054,44 +4032,20 @@
       'driver':         'BlockdevDriver' },
   'discriminator': 'driver',
   'data': {
-      'blkdebug':       'BlockdevCreateNotSupported',
-      'blkverify':      'BlockdevCreateNotSupported',
-      'bochs':          'BlockdevCreateNotSupported',
-      'cloop':          'BlockdevCreateNotSupported',
-      'copy-on-read':   'BlockdevCreateNotSupported',
-      'dmg':            'BlockdevCreateNotSupported',
       'file':           'BlockdevCreateOptionsFile',
-      'ftp':            'BlockdevCreateNotSupported',
-      'ftps':           'BlockdevCreateNotSupported',
       'gluster':        'BlockdevCreateOptionsGluster',
-      'host_cdrom':     'BlockdevCreateNotSupported',
-      'host_device':    'BlockdevCreateNotSupported',
-      'http':           'BlockdevCreateNotSupported',
-      'https':          'BlockdevCreateNotSupported',
-      'iscsi':          'BlockdevCreateNotSupported',
       'luks':           'BlockdevCreateOptionsLUKS',
-      'nbd':            'BlockdevCreateNotSupported',
       'nfs':            'BlockdevCreateOptionsNfs',
-      'null-aio':       'BlockdevCreateNotSupported',
-      'null-co':        'BlockdevCreateNotSupported',
-      'nvme':           'BlockdevCreateNotSupported',
       'parallels':      'BlockdevCreateOptionsParallels',
       'qcow':           'BlockdevCreateOptionsQcow',
       'qcow2':          'BlockdevCreateOptionsQcow2',
       'qed':            'BlockdevCreateOptionsQed',
-      'quorum':         'BlockdevCreateNotSupported',
-      'raw':            'BlockdevCreateNotSupported',
       'rbd':            'BlockdevCreateOptionsRbd',
-      'replication':    'BlockdevCreateNotSupported',
       'sheepdog':       'BlockdevCreateOptionsSheepdog',
       'ssh':            'BlockdevCreateOptionsSsh',
-      'throttle':       'BlockdevCreateNotSupported',
       'vdi':            'BlockdevCreateOptionsVdi',
       'vhdx':           'BlockdevCreateOptionsVhdx',
-      'vmdk':           'BlockdevCreateNotSupported',
-      'vpc':            'BlockdevCreateOptionsVpc',
-      'vvfat':          'BlockdevCreateNotSupported',
-      'vxhs':           'BlockdevCreateNotSupported'
+      'vpc':            'BlockdevCreateOptionsVpc'
   } }
 
 ##
diff --git a/qapi/crypto.json b/qapi/crypto.json
index 288bc05..a51b434 100644
--- a/qapi/crypto.json
+++ b/qapi/crypto.json
@@ -297,16 +297,6 @@
            'uuid': 'str',
            'slots': [ 'QCryptoBlockInfoLUKSSlot' ] }}
 
-##
-# @QCryptoBlockInfoQCow:
-#
-# Information about the QCow block encryption options
-#
-# Since: 2.7
-##
-{ 'struct': 'QCryptoBlockInfoQCow',
-  'data': { }}
-
 
 ##
 # @QCryptoBlockInfo:
@@ -318,5 +308,4 @@
 { 'union': 'QCryptoBlockInfo',
   'base': 'QCryptoBlockInfoBase',
   'discriminator': 'format',
-  'data': { 'qcow': 'QCryptoBlockInfoQCow',
-            'luks': 'QCryptoBlockInfoLUKS' } }
+  'data': { 'luks': 'QCryptoBlockInfoLUKS' } }
diff --git a/qapi/misc.json b/qapi/misc.json
index f83a63a..39f4dc1 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -395,8 +395,7 @@
             'mips': 'CpuInfoMIPS',
             'tricore': 'CpuInfoTricore',
             's390': 'CpuInfoS390',
-            'riscv': 'CpuInfoRISCV',
-            'other': 'CpuInfoOther' } }
+            'riscv': 'CpuInfoRISCV' } }
 
 ##
 # @CpuInfoX86:
@@ -467,16 +466,6 @@
 { 'struct': 'CpuInfoRISCV', 'data': { 'pc': 'int' } }
 
 ##
-# @CpuInfoOther:
-#
-# No additional information is available about the virtual CPU
-#
-# Since: 2.6
-#
-##
-{ 'struct': 'CpuInfoOther', 'data': { } }
-
-##
 # @CpuS390State:
 #
 # An enumeration of cpu states that can be assumed by a virtual
@@ -577,38 +566,7 @@
                       'arch'         : 'CpuInfoArch',
                       'target'       : 'SysEmuTarget' },
   'discriminator' : 'target',
-  'data'          : { 'aarch64'      : 'CpuInfoOther',
-                      'alpha'        : 'CpuInfoOther',
-                      'arm'          : 'CpuInfoOther',
-                      'cris'         : 'CpuInfoOther',
-                      'hppa'         : 'CpuInfoOther',
-                      'i386'         : 'CpuInfoOther',
-                      'lm32'         : 'CpuInfoOther',
-                      'm68k'         : 'CpuInfoOther',
-                      'microblaze'   : 'CpuInfoOther',
-                      'microblazeel' : 'CpuInfoOther',
-                      'mips'         : 'CpuInfoOther',
-                      'mips64'       : 'CpuInfoOther',
-                      'mips64el'     : 'CpuInfoOther',
-                      'mipsel'       : 'CpuInfoOther',
-                      'moxie'        : 'CpuInfoOther',
-                      'nios2'        : 'CpuInfoOther',
-                      'or1k'         : 'CpuInfoOther',
-                      'ppc'          : 'CpuInfoOther',
-                      'ppc64'        : 'CpuInfoOther',
-                      'ppcemb'       : 'CpuInfoOther',
-                      'riscv32'      : 'CpuInfoOther',
-                      'riscv64'      : 'CpuInfoOther',
-                      's390x'        : 'CpuInfoS390',
-                      'sh4'          : 'CpuInfoOther',
-                      'sh4eb'        : 'CpuInfoOther',
-                      'sparc'        : 'CpuInfoOther',
-                      'sparc64'      : 'CpuInfoOther',
-                      'tricore'      : 'CpuInfoOther',
-                      'unicore32'    : 'CpuInfoOther',
-                      'x86_64'       : 'CpuInfoOther',
-                      'xtensa'       : 'CpuInfoOther',
-                      'xtensaeb'     : 'CpuInfoOther' } }
+  'data'          : { 's390x'        : 'CpuInfoS390' } }
 
 ##
 # @query-cpus-fast:
diff --git a/qapi/net.json b/qapi/net.json
index 6b7d93c..c86f351 100644
--- a/qapi/net.json
+++ b/qapi/net.json
@@ -89,16 +89,6 @@
 { 'command': 'netdev_del', 'data': {'id': 'str'} }
 
 ##
-# @NetdevNoneOptions:
-#
-# Use it alone to have zero network devices.
-#
-# Since: 1.2
-##
-{ 'struct': 'NetdevNoneOptions',
-  'data': { } }
-
-##
 # @NetLegacyNicOptions:
 #
 # Create a new Network Interface Card.
@@ -477,7 +467,6 @@
   'base': { 'id': 'str', 'type': 'NetClientDriver' },
   'discriminator': 'type',
   'data': {
-    'none':     'NetdevNoneOptions',
     'nic':      'NetLegacyNicOptions',
     'user':     'NetdevUserOptions',
     'tap':      'NetdevTapOptions',
@@ -530,7 +519,6 @@
   'base': { 'type': 'NetLegacyOptionsType' },
   'discriminator': 'type',
   'data': {
-    'none':     'NetdevNoneOptions',
     'nic':      'NetLegacyNicOptions',
     'user':     'NetdevUserOptions',
     'tap':      'NetdevTapOptions',
diff --git a/qapi/ui.json b/qapi/ui.json
index fc18a05..f48d2a0 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -996,17 +996,6 @@
 
 
 ##
-# @DisplayNoOpts:
-#
-# Empty struct for displays without config options.
-#
-# Since: 2.12
-#
-##
-{ 'struct'  : 'DisplayNoOpts',
-  'data'    : { } }
-
-##
 # @DisplayGTK:
 #
 # GTK display options.
@@ -1068,10 +1057,4 @@
                 '*window-close'  : 'bool',
                 '*gl'            : 'DisplayGLMode' },
   'discriminator' : 'type',
-  'data'    : { 'default'        : 'DisplayNoOpts',
-                'none'           : 'DisplayNoOpts',
-                'gtk'            : 'DisplayGTK',
-                'sdl'            : 'DisplayNoOpts',
-                'egl-headless'   : 'DisplayNoOpts',
-                'curses'         : 'DisplayNoOpts',
-                'cocoa'          : 'DisplayNoOpts' } }
+  'data'    : { 'gtk'            : 'DisplayGTK' } }
diff --git a/block/qcow2.c b/block/qcow2.c
index 6fa5e1d..b44e100 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4169,7 +4169,6 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
         switch (encrypt_info->format) {
         case Q_CRYPTO_BLOCK_FORMAT_QCOW:
             qencrypt->format = BLOCKDEV_QCOW2_ENCRYPTION_FORMAT_AES;
-            qencrypt->u.aes = encrypt_info->u.qcow;
             break;
         case Q_CRYPTO_BLOCK_FORMAT_LUKS:
             qencrypt->format = BLOCKDEV_QCOW2_ENCRYPTION_FORMAT_LUKS;
diff --git a/cpus.c b/cpus.c
index d1f1629..19c5d37 100644
--- a/cpus.c
+++ b/cpus.c
@@ -2273,8 +2273,6 @@ CpuInfoFastList *qmp_query_cpus_fast(Error **errp)
         info->value->target = target;
         if (target == SYS_EMU_TARGET_S390X) {
             cpustate_to_cpuinfo_s390(&info->value->u.s390x, cpu);
-        } else {
-            /* do nothing for @CpuInfoOther */
         }
 
         if (!cur_item) {
-- 
2.7.4


Re: [Qemu-devel] [PATCH v4 2/2] qapi: remove empty flat union branches and types
Posted by Markus Armbruster, 1 week ago
Anton Nefedov <anton.nefedov@virtuozzo.com> writes:

> Flat unions may now have uncovered branches, so it is possible to get rid
> of empty types defined for that purpose only.
>
> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>

I observe a nice shrinking of generated code:

 qapi-doc.texi           |  134 -----
 qapi-introspect.c       | 1124 +++++++++++++++++++++++-------------------------
 qapi-types-block-core.c |   26 -
 qapi-types-block-core.h |   43 -
 qapi-types-crypto.c     |   13 
 qapi-types-crypto.h     |    9 
 qapi-types-misc.c       |   13 
 qapi-types-misc.h       |   40 -
 qapi-types-net.c        |   13 
 qapi-types-net.h        |   10 
 qapi-types-ui.c         |   13 
 qapi-types-ui.h         |   14 
 qapi-visit-block-core.c |  185 +------
 qapi-visit-block-core.h |    6 
 qapi-visit-core.d       |   15 
 qapi-visit-crypto.c     |   38 -
 qapi-visit-crypto.h     |    3 
 qapi-visit-misc.c       |   71 ---
 qapi-visit-misc.h       |    3 
 qapi-visit-net.c        |   43 -
 qapi-visit-net.h        |    3 
 qapi-visit-ui.c         |   45 -
 qapi-visit-ui.h         |    3 
 23 files changed, 607 insertions(+), 1260 deletions(-)

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