* [Qemu-devel] [PATCH v3 1/2] qapi: allow empty branches in flat unions
2018-05-30 16:37 [Qemu-devel] [PATCH v3 0/2] qapi: allow empty branches in flat unions Anton Nefedov
@ 2018-05-30 16:37 ` Anton Nefedov
2018-06-01 21:45 ` Eric Blake
2018-05-30 16:37 ` [Qemu-devel] [PATCH v3 2/2] qapi: remove empty flat union branches and types Anton Nefedov
1 sibling, 1 reply; 4+ messages in thread
From: Anton Nefedov @ 2018-05-30 16:37 UTC (permalink / raw)
To: qemu-devel; +Cc: eblake, armbru, mdroth, Anton Nefedov
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>
---
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 b9b6eab..3dfedd5 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 06e30f4..7ceb409 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 a032cec..df6190a 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -782,13 +782,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 b499ba1..1f1e0ea 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -494,7 +494,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 467577d..9dc9532 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
^ permalink raw reply related [flat|nested] 4+ messages in thread* [Qemu-devel] [PATCH v3 2/2] qapi: remove empty flat union branches and types
2018-05-30 16:37 [Qemu-devel] [PATCH v3 0/2] qapi: allow empty branches in flat unions Anton Nefedov
2018-05-30 16:37 ` [Qemu-devel] [PATCH v3 1/2] " Anton Nefedov
@ 2018-05-30 16:37 ` Anton Nefedov
1 sibling, 0 replies; 4+ messages in thread
From: Anton Nefedov @ 2018-05-30 16:37 UTC (permalink / raw)
To: qemu-devel; +Cc: eblake, armbru, mdroth, Anton Nefedov
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>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
qapi/block-core.json | 55 ++++------------------------------------------------
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, 8 insertions(+), 140 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index ad66ad6..d0b95f0 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:
@@ -2671,8 +2670,7 @@
{ 'union': 'BlockdevQcow2Encryption',
'base': { 'format': 'BlockdevQcow2EncryptionFormat' },
'discriminator': 'format',
- 'data': { 'aes': 'QCryptoBlockOptionsQCow',
- 'luks': 'QCryptoBlockOptionsLUKS'} }
+ 'data': { 'luks': 'QCryptoBlockOptionsLUKS'} }
##
# @BlockdevOptionsQcow2:
@@ -2769,16 +2767,6 @@
'hash': 'str' }}
##
-# @SshHostKeyDummy:
-#
-# For those union branches that don't need additional fields.
-#
-# Since: 2.12
-##
-{ 'struct': 'SshHostKeyDummy',
- 'data': {} }
-
-##
# @SshHostKeyCheck:
#
# Since: 2.12
@@ -2786,9 +2774,7 @@
{ 'union': 'SshHostKeyCheck',
'base': { 'mode': 'SshHostKeyCheckMode' },
'discriminator': 'mode',
- 'data': { 'none': 'SshHostKeyDummy',
- 'hash': 'SshHostKeyHash',
- 'known_hosts': 'SshHostKeyDummy' } }
+ 'data': { 'hash': 'SshHostKeyHash' } }
##
# @BlockdevOptionsSsh:
@@ -3948,15 +3934,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.
@@ -3970,44 +3947,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 99bcaac..a46e55c 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -393,8 +393,7 @@
'mips': 'CpuInfoMIPS',
'tricore': 'CpuInfoTricore',
's390': 'CpuInfoS390',
- 'riscv': 'CpuInfoRISCV',
- 'other': 'CpuInfoOther' } }
+ 'riscv': 'CpuInfoRISCV' } }
##
# @CpuInfoX86:
@@ -465,16 +464,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
@@ -575,38 +564,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 5c1dc48..3a43950 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.
@@ -473,7 +463,6 @@
'base': { 'id': 'str', 'type': 'NetClientDriver' },
'discriminator': 'type',
'data': {
- 'none': 'NetdevNoneOptions',
'nic': 'NetLegacyNicOptions',
'user': 'NetdevUserOptions',
'tap': 'NetdevTapOptions',
@@ -526,7 +515,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 6d53247..389d96d 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4003,7 +4003,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
^ permalink raw reply related [flat|nested] 4+ messages in thread