[Qemu-devel] [PATCH v2 00/10] block: Try to create well typed json:{} filenames

[Qemu-devel] [PATCH v2 00/10] block: Try to create well typed json:{} filenames

[Max Reitz]

See cover letter of v1 here:
http://lists.nongnu.org/archive/html/qemu-block/2018-05/msg00290.html


This series depends on Markus's series "block: Configuration fixes and
rbd authentication":

Based-on: <20180607062559.16127-1-armbru@redhat.com>


Changes in v2:
- Rebased on top of Markus's series, which allowed my to drop
  qdict_stringify_for_keyval(), and the patch moving block-specific
  QDict functions into an own header
- %s/2.13/3.0/g

- Patch 1: [Eric]
  - s/must/will/
  - Set the discriminator field to its default value when not given,
    which simplifies the code both here and hopefully in the users, too
- Patch 3: [Eric]
  - Rename existing optional discriminator test and keep it as an error
    test, instead of making it pass
  - Add passing test to qapi-schema-test
- Patch 4: [Daniel]
  - %s/from-image/auto/g
  - Actually handle that format when specified explicitly
- Patch 5: [Eric/Daniel]
  - Reduced complexity by just defaulting to 'aes'
- Patch 6: Rewritten because of the rebase
- Patch 7: Rebase conflicts
- Patch 8:
  - Use qobject_input_visitor_new_flat_confused() from Markus's series
  - Some rebase conflicts in test outputs
  - Also, 198 now explicitly emits '"format": "auto"' due to visitors
    now setting the discriminator field to the default value, thanks to
    the change to patch 1 (working as intended, I suppose)
- Patch 10: Added a couple of qcow2+LUKS blockdev-add test cases for
            various values of encrypt.format


git-backport-diff against v1:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/10:[0032] [FC] 'qapi: Add default-variant for flat unions'
002/10:[----] [--] 'docs/qapi: Document optional discriminators'
003/10:[0048] [FC] 'tests: Add QAPI optional discriminator tests'
004/10:[0025] [FC] 'qapi: Formalize qcow2 encryption probing'
005/10:[0027] [FC] 'qapi: Formalize qcow encryption probing'
006/10:[0042] [FC] 'qdict: Make qdict_flatten() shallow-clone-friendly'
007/10:[0002] [FC] 'tests: Add QDict clone-flatten test'
008/10:[0035] [FC] 'block: Try to create well typed json:{} filenames'
009/10:[----] [--] 'iotests: Test internal option typing'
010/10:[0089] [FC] 'iotests: qcow2's encrypt.format is now optional'


Max Reitz (10):
  qapi: Add default-variant for flat unions
  docs/qapi: Document optional discriminators
  tests: Add QAPI optional discriminator tests
  qapi: Formalize qcow2 encryption probing
  qapi: Formalize qcow encryption probing
  qdict: Make qdict_flatten() shallow-clone-friendly
  tests: Add QDict clone-flatten test
  block: Try to create well typed json:{} filenames
  iotests: Test internal option typing
  iotests: qcow2's encrypt.format is now optional

 docs/devel/qapi-code-gen.txt                  | 21 +++++-
 tests/Makefile.include                        |  5 +-
 qapi/block-core.json                          | 47 +++++++++++--
 qapi/introspect.json                          |  8 +++
 ...ptional-discriminator-invalid-default.json | 12 ++++
 ...l-discriminator-invalid-specification.json | 12 ++++
 ...on-optional-discriminator-no-default.json} |  5 +-
 ...lat-union-superfluous-default-variant.json | 11 ++++
 tests/qapi-schema/qapi-schema-test.json       |  9 +++
 block.c                                       | 66 ++++++++++++++++++-
 block/qcow2.c                                 |  3 +
 qobject/block-qdict.c                         | 19 ++++--
 tests/check-block-qdict.c                     | 33 ++++++++++
 scripts/qapi/common.py                        | 57 +++++++++++++---
 scripts/qapi/doc.py                           |  8 ++-
 scripts/qapi/introspect.py                    | 10 ++-
 scripts/qapi/visit.py                         | 13 ++++
 ...optional-discriminator-invalid-default.err |  1 +
 ...tional-discriminator-invalid-default.exit} |  0
 ...ptional-discriminator-invalid-default.out} |  0
 ...al-discriminator-invalid-specification.err |  1 +
 ...l-discriminator-invalid-specification.exit |  1 +
 ...al-discriminator-invalid-specification.out |  0
 ...nion-optional-discriminator-no-default.err |  1 +
 ...ion-optional-discriminator-no-default.exit |  1 +
 ...nion-optional-discriminator-no-default.out |  0
 .../flat-union-optional-discriminator.err     |  1 -
 ...flat-union-superfluous-default-variant.err |  1 +
 ...lat-union-superfluous-default-variant.exit |  1 +
 ...flat-union-superfluous-default-variant.out |  0
 tests/qapi-schema/qapi-schema-test.out        |  9 +++
 tests/qapi-schema/test-qapi.py                |  2 +
 tests/qemu-iotests/059.out                    |  2 +-
 tests/qemu-iotests/087                        | 65 ++++++++++--------
 tests/qemu-iotests/087.out                    | 26 +++++++-
 tests/qemu-iotests/089                        | 25 +++++++
 tests/qemu-iotests/089.out                    |  9 +++
 tests/qemu-iotests/099.out                    |  4 +-
 tests/qemu-iotests/110.out                    |  2 +-
 tests/qemu-iotests/198.out                    |  4 +-
 tests/qemu-iotests/207.out                    | 10 +--
 41 files changed, 436 insertions(+), 69 deletions(-)
 create mode 100644 tests/qapi-schema/flat-union-optional-discriminator-invalid-default.json
 create mode 100644 tests/qapi-schema/flat-union-optional-discriminator-invalid-specification.json
 rename tests/qapi-schema/{flat-union-optional-discriminator.json => flat-union-optional-discriminator-no-default.json} (70%)
 create mode 100644 tests/qapi-schema/flat-union-superfluous-default-variant.json
 create mode 100644 tests/qapi-schema/flat-union-optional-discriminator-invalid-default.err
 rename tests/qapi-schema/{flat-union-optional-discriminator.exit => flat-union-optional-discriminator-invalid-default.exit} (100%)
 rename tests/qapi-schema/{flat-union-optional-discriminator.out => flat-union-optional-discriminator-invalid-default.out} (100%)
 create mode 100644 tests/qapi-schema/flat-union-optional-discriminator-invalid-specification.err
 create mode 100644 tests/qapi-schema/flat-union-optional-discriminator-invalid-specification.exit
 create mode 100644 tests/qapi-schema/flat-union-optional-discriminator-invalid-specification.out
 create mode 100644 tests/qapi-schema/flat-union-optional-discriminator-no-default.err
 create mode 100644 tests/qapi-schema/flat-union-optional-discriminator-no-default.exit
 create mode 100644 tests/qapi-schema/flat-union-optional-discriminator-no-default.out
 delete mode 100644 tests/qapi-schema/flat-union-optional-discriminator.err
 create mode 100644 tests/qapi-schema/flat-union-superfluous-default-variant.err
 create mode 100644 tests/qapi-schema/flat-union-superfluous-default-variant.exit
 create mode 100644 tests/qapi-schema/flat-union-superfluous-default-variant.out

-- 
2.17.1

[Qemu-devel] [PATCH v2 01/10] qapi: Add default-variant for flat unions

[Max Reitz]

This patch allows specifying a discriminator that is an optional member
of the base struct.  In such a case, a default value must be provided
that is used when no value is given.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 qapi/introspect.json           |  8 +++++
 scripts/qapi/common.py         | 57 ++++++++++++++++++++++++++++------
 scripts/qapi/doc.py            |  8 +++--
 scripts/qapi/introspect.py     | 10 ++++--
 scripts/qapi/visit.py          | 13 ++++++++
 tests/qapi-schema/test-qapi.py |  2 ++
 6 files changed, 83 insertions(+), 15 deletions(-)

diff --git a/qapi/introspect.json b/qapi/introspect.json
index 80a0a3e656..b43c87fe8d 100644
--- a/qapi/introspect.json
+++ b/qapi/introspect.json
@@ -168,6 +168,13 @@
 # @tag: the name of the member serving as type tag.
 #       An element of @members with this name must exist.
 #
+# @default-variant: if the @tag element of @members is optional, this
+#                   is the default value for choosing a variant.  Its
+#                   value will be a valid value for @tag.
+#                   Present exactly when @tag is present and the
+#                   associated element of @members is optional.
+#                   (Since: 3.0)
+#
 # @variants: variant members, i.e. additional members that
 #            depend on the type tag's value.  Present exactly when
 #            @tag is present.  The variants are in no particular order,
@@ -181,6 +188,7 @@
 { 'struct': 'SchemaInfoObject',
   'data': { 'members': [ 'SchemaInfoObjectMember' ],
             '*tag': 'str',
+            '*default-variant': 'str',
             '*variants': [ 'SchemaInfoObjectVariant' ] } }
 
 ##
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index e82990f0f2..d15f56b260 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -721,6 +721,7 @@ def check_union(expr, info):
     name = expr['union']
     base = expr.get('base')
     discriminator = expr.get('discriminator')
+    default_variant = expr.get('default-variant')
     members = expr['data']
 
     # Two types of unions, determined by discriminator.
@@ -745,16 +746,37 @@ def check_union(expr, info):
         base_members = find_base_members(base)
         assert base_members is not None
 
-        # The value of member 'discriminator' must name a non-optional
-        # member of the base struct.
+        # The value of member 'discriminator' must name a member of
+        # the base struct.
         check_name(info, "Discriminator of flat union '%s'" % name,
                    discriminator)
-        discriminator_type = base_members.get(discriminator)
-        if not discriminator_type:
-            raise QAPISemError(info,
-                               "Discriminator '%s' is not a member of base "
-                               "struct '%s'"
-                               % (discriminator, base))
+        if default_variant is None:
+            discriminator_type = base_members.get(discriminator)
+            if not discriminator_type:
+                if base_members.get('*' + discriminator) is None:
+                    raise QAPISemError(info,
+                                       "Discriminator '%s' is not a member of "
+                                       "base struct '%s'"
+                                       % (discriminator, base))
+                else:
+                    raise QAPISemError(info,
+                                       "Default variant must be specified for "
+                                       "optional discriminator '%s'"
+                                       % discriminator)
+        else:
+            discriminator_type = base_members.get('*' + discriminator)
+            if not discriminator_type:
+                if base_members.get(discriminator) is None:
+                    raise QAPISemError(info,
+                                       "Discriminator '%s' is not a member of "
+                                       "base struct '%s'"
+                                       % (discriminator, base))
+                else:
+                    raise QAPISemError(info,
+                                       "Must not specify a default variant for "
+                                       "non-optional discriminator '%s'"
+                                       % discriminator)
+
         enum_define = enum_types.get(discriminator_type)
         allow_metas = ['struct']
         # Do not allow string discriminator
@@ -763,6 +785,15 @@ def check_union(expr, info):
                                "Discriminator '%s' must be of enumeration "
                                "type" % discriminator)
 
+        if default_variant is not None:
+            # Must be a value of the enumeration
+            if default_variant not in enum_define['data']:
+                raise QAPISemError(info,
+                                   "Default variant '%s' of flat union '%s' is "
+                                   "not part of '%s'"
+                                   % (default_variant, name,
+                                      discriminator_type))
+
     # Check every branch; don't allow an empty union
     if len(members) == 0:
         raise QAPISemError(info, "Union '%s' cannot have empty 'data'" % name)
@@ -910,7 +941,7 @@ def check_exprs(exprs):
         elif 'union' in expr:
             meta = 'union'
             check_keys(expr_elem, 'union', ['data'],
-                       ['base', 'discriminator'])
+                       ['base', 'discriminator', 'default-variant'])
             union_types[expr[meta]] = expr
         elif 'alternate' in expr:
             meta = 'alternate'
@@ -1336,12 +1367,14 @@ class QAPISchemaObjectTypeMember(QAPISchemaMember):
 
 
 class QAPISchemaObjectTypeVariants(object):
-    def __init__(self, tag_name, tag_member, variants):
+    def __init__(self, tag_name, tag_member, default_tag_value, variants):
         # Flat unions pass tag_name but not tag_member.
         # Simple unions and alternates pass tag_member but not tag_name.
         # After check(), tag_member is always set, and tag_name remains
         # a reliable witness of being used by a flat union.
         assert bool(tag_member) != bool(tag_name)
+        # default_tag_value is only passed for flat unions.
+        assert bool(tag_name) or not bool(default_tag_value)
         assert (isinstance(tag_name, str) or
                 isinstance(tag_member, QAPISchemaObjectTypeMember))
         assert len(variants) > 0
@@ -1349,6 +1382,7 @@ class QAPISchemaObjectTypeVariants(object):
             assert isinstance(v, QAPISchemaObjectTypeVariant)
         self._tag_name = tag_name
         self.tag_member = tag_member
+        self.default_tag_value = default_tag_value
         self.variants = variants
 
     def set_owner(self, name):
@@ -1640,6 +1674,7 @@ class QAPISchema(object):
         data = expr['data']
         base = expr.get('base')
         tag_name = expr.get('discriminator')
+        default_tag_value = expr.get('default-variant')
         tag_member = None
         if isinstance(base, dict):
             base = (self._make_implicit_object_type(
@@ -1659,6 +1694,7 @@ class QAPISchema(object):
             QAPISchemaObjectType(name, info, doc, base, members,
                                  QAPISchemaObjectTypeVariants(tag_name,
                                                               tag_member,
+                                                              default_tag_value,
                                                               variants)))
 
     def _def_alternate_type(self, expr, info, doc):
@@ -1671,6 +1707,7 @@ class QAPISchema(object):
             QAPISchemaAlternateType(name, info, doc,
                                     QAPISchemaObjectTypeVariants(None,
                                                                  tag_member,
+                                                                 None,
                                                                  variants)))
 
     def _def_command(self, expr, info, doc):
diff --git a/scripts/qapi/doc.py b/scripts/qapi/doc.py
index b5630844f9..7dd14173e5 100644
--- a/scripts/qapi/doc.py
+++ b/scripts/qapi/doc.py
@@ -160,8 +160,12 @@ def texi_members(doc, what, base, variants, member_func):
         items += '@item The members of @code{%s}\n' % base.doc_type()
     if variants:
         for v in variants.variants:
-            when = ' when @code{%s} is @t{"%s"}' % (
-                variants.tag_member.name, v.name)
+            if v.name == variants.default_tag_value:
+                when = ' when @code{%s} is @t{"%s"} or not given' % (
+                    variants.tag_member.name, v.name)
+            else:
+                when = ' when @code{%s} is @t{"%s"}' % (
+                    variants.tag_member.name, v.name)
             if v.type.is_implicit():
                 assert not v.type.base and not v.type.variants
                 for m in v.type.local_members:
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 5b6c72c7b2..0133f91792 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -142,9 +142,12 @@ const QLitObject %(c_name)s = %(c_string)s;
             ret['default'] = None
         return ret
 
-    def _gen_variants(self, tag_name, variants):
-        return {'tag': tag_name,
-                'variants': [self._gen_variant(v) for v in variants]}
+    def _gen_variants(self, tag_name, default_variant, variants):
+        ret = {'tag': tag_name,
+               'variants': [self._gen_variant(v) for v in variants]}
+        if default_variant:
+            ret['default-variant'] = default_variant
+        return ret
 
     def _gen_variant(self, variant):
         return {'case': variant.name, 'type': self._use_type(variant.type)}
@@ -163,6 +166,7 @@ const QLitObject %(c_name)s = %(c_string)s;
         obj = {'members': [self._gen_member(m) for m in members]}
         if variants:
             obj.update(self._gen_variants(variants.tag_member.name,
+                                          variants.default_tag_value,
                                           variants.variants))
         self._gen_qlit(name, 'object', obj)
 
diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
index 5d72d8936c..4c432f3ead 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -75,6 +75,19 @@ void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp)
 ''')
 
     if variants:
+        if variants.default_tag_value is not None:
+            ret += mcgen('''
+    if (!obj->has_%(c_name)s) {
+        obj->has_%(c_name)s = true;
+        obj->%(c_name)s = %(enum_const)s;
+    }
+''',
+                         c_name=c_name(variants.tag_member.name),
+                         enum_const=c_enum_const(
+                             variants.tag_member.type.name,
+                             variants.default_tag_value,
+                             variants.tag_member.type.prefix))
+
         ret += mcgen('''
     switch (obj->%(c_name)s) {
 ''',
diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
index 4512a41504..3609e98348 100644
--- a/tests/qapi-schema/test-qapi.py
+++ b/tests/qapi-schema/test-qapi.py
@@ -56,6 +56,8 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):
     def _print_variants(variants):
         if variants:
             print('    tag %s' % variants.tag_member.name)
+            if variants.default_tag_value:
+                print('    default variant: %s' % variants.default_tag_value)
             for v in variants.variants:
                 print('    case %s: %s' % (v.name, v.type.name))
 
-- 
2.17.1

[Qemu-devel] [PATCH v2 02/10] docs/qapi: Document optional discriminators

[Max Reitz]

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 docs/devel/qapi-code-gen.txt | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
index 1366228b2a..f0a2101096 100644
--- a/docs/devel/qapi-code-gen.txt
+++ b/docs/devel/qapi-code-gen.txt
@@ -472,8 +472,8 @@ All branches of the union must be complex types, and the top-level
 members of the union dictionary on the wire will be combination of
 members from both the base type and the appropriate branch type (when
 merging two dictionaries, there must be no keys in common).  The
-'discriminator' member must be the name of a non-optional enum-typed
-member of the base struct.
+'discriminator' member must be the name of an enum-typed member of the
+base struct.
 
 The following example enhances the above simple union example by
 adding an optional common member 'read-only', renaming the
@@ -502,6 +502,23 @@ the enum).  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.
 
+If the discriminator points to an optional member of the base struct,
+its default value must be specified as a 'default-variant'.  In the
+following example, the above BlockDriver struct is changed so it
+defaults to the 'file' driver if that field is omitted on the wire:
+
+ { 'union': 'BlockdevOptions',
+   'base': { '*driver': 'BlockdevDriver', '*read-only': 'bool' },
+   'discriminator': 'driver',
+   'default-variant': 'file',
+   'data': { 'file': 'BlockdevOptionsFile',
+             'qcow2': 'BlockdevOptionsQcow2' } }
+
+Now the 'file' JSON object can be abbreviated to:
+
+ { "read-only": "true",
+   "filename": "/some/place/my-image" }
+
 A simple union can always be re-written as a flat union where the base
 class has a single member named 'type', and where each branch of the
 union has a struct with a single member named 'data'.  That is,
-- 
2.17.1

[Qemu-devel] [PATCH v2 03/10] tests: Add QAPI optional discriminator tests

[Max Reitz]

There already is an optional discriminator test, although it also noted
the discriminator name itself as optional.  This already gives us one
error test case, to which this patch adds various others.

Furthermore, a passing test case is added to qapi-schema-test.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/Makefile.include                               |  5 ++++-
 ...union-optional-discriminator-invalid-default.json | 12 ++++++++++++
 ...optional-discriminator-invalid-specification.json | 12 ++++++++++++
 ...lat-union-optional-discriminator-no-default.json} |  5 +++--
 .../flat-union-superfluous-default-variant.json      | 11 +++++++++++
 tests/qapi-schema/qapi-schema-test.json              |  9 +++++++++
 ...-union-optional-discriminator-invalid-default.err |  1 +
 ...nion-optional-discriminator-invalid-default.exit} |  0
 ...union-optional-discriminator-invalid-default.out} |  0
 ...-optional-discriminator-invalid-specification.err |  1 +
 ...optional-discriminator-invalid-specification.exit |  1 +
 ...-optional-discriminator-invalid-specification.out |  0
 .../flat-union-optional-discriminator-no-default.err |  1 +
 ...flat-union-optional-discriminator-no-default.exit |  1 +
 .../flat-union-optional-discriminator-no-default.out |  0
 .../flat-union-optional-discriminator.err            |  1 -
 .../flat-union-superfluous-default-variant.err       |  1 +
 .../flat-union-superfluous-default-variant.exit      |  1 +
 .../flat-union-superfluous-default-variant.out       |  0
 tests/qapi-schema/qapi-schema-test.out               |  9 +++++++++
 20 files changed, 67 insertions(+), 4 deletions(-)
 create mode 100644 tests/qapi-schema/flat-union-optional-discriminator-invalid-default.json
 create mode 100644 tests/qapi-schema/flat-union-optional-discriminator-invalid-specification.json
 rename tests/qapi-schema/{flat-union-optional-discriminator.json => flat-union-optional-discriminator-no-default.json} (70%)
 create mode 100644 tests/qapi-schema/flat-union-superfluous-default-variant.json
 create mode 100644 tests/qapi-schema/flat-union-optional-discriminator-invalid-default.err
 rename tests/qapi-schema/{flat-union-optional-discriminator.exit => flat-union-optional-discriminator-invalid-default.exit} (100%)
 rename tests/qapi-schema/{flat-union-optional-discriminator.out => flat-union-optional-discriminator-invalid-default.out} (100%)
 create mode 100644 tests/qapi-schema/flat-union-optional-discriminator-invalid-specification.err
 create mode 100644 tests/qapi-schema/flat-union-optional-discriminator-invalid-specification.exit
 create mode 100644 tests/qapi-schema/flat-union-optional-discriminator-invalid-specification.out
 create mode 100644 tests/qapi-schema/flat-union-optional-discriminator-no-default.err
 create mode 100644 tests/qapi-schema/flat-union-optional-discriminator-no-default.exit
 create mode 100644 tests/qapi-schema/flat-union-optional-discriminator-no-default.out
 delete mode 100644 tests/qapi-schema/flat-union-optional-discriminator.err
 create mode 100644 tests/qapi-schema/flat-union-superfluous-default-variant.err
 create mode 100644 tests/qapi-schema/flat-union-superfluous-default-variant.exit
 create mode 100644 tests/qapi-schema/flat-union-superfluous-default-variant.out

diff --git a/tests/Makefile.include b/tests/Makefile.include
index 2023e257fd..66d10e1072 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -505,8 +505,11 @@ 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-no-base.json
-qapi-schema += flat-union-optional-discriminator.json
+qapi-schema += flat-union-optional-discriminator-invalid-specification.json
+qapi-schema += flat-union-optional-discriminator-no-default.json
+qapi-schema += flat-union-optional-discriminator-invalid-default.json
 qapi-schema += flat-union-string-discriminator.json
+qapi-schema += flat-union-superfluous-default-variant.json
 qapi-schema += funny-char.json
 qapi-schema += ident-with-escape.json
 qapi-schema += include-before-err.json
diff --git a/tests/qapi-schema/flat-union-optional-discriminator-invalid-default.json b/tests/qapi-schema/flat-union-optional-discriminator-invalid-default.json
new file mode 100644
index 0000000000..015a47ba52
--- /dev/null
+++ b/tests/qapi-schema/flat-union-optional-discriminator-invalid-default.json
@@ -0,0 +1,12 @@
+# default-variant must refer to an actual value of the discriminator's
+# enum
+{ 'enum': 'Enum', 'data': [ 'one', 'two' ] }
+{ 'struct': 'Base',
+  'data': { '*switch': 'Enum' } }
+{ 'struct': 'Branch', 'data': { 'name': 'str' } }
+{ 'union': 'MyUnion',
+  'base': 'Base',
+  'discriminator': 'switch',
+  'default-variant': 'three',
+  'data': { 'one': 'Branch',
+            'two': 'Branch' } }
diff --git a/tests/qapi-schema/flat-union-optional-discriminator-invalid-specification.json b/tests/qapi-schema/flat-union-optional-discriminator-invalid-specification.json
new file mode 100644
index 0000000000..fd896942a2
--- /dev/null
+++ b/tests/qapi-schema/flat-union-optional-discriminator-invalid-specification.json
@@ -0,0 +1,12 @@
+# For using optional discriminators, only the field in the base struct
+# must be marked optional, not the discriminator name itself
+{ 'enum': 'Enum', 'data': [ 'one', 'two' ] }
+{ 'struct': 'Base',
+  'data': { '*switch': 'Enum' } }
+{ 'struct': 'Branch', 'data': { 'name': 'str' } }
+{ 'union': 'MyUnion',
+  'base': 'Base',
+  'discriminator': '*switch',
+  'default-variant': 'one',
+  'data': { 'one': 'Branch',
+            'two': 'Branch' } }
diff --git a/tests/qapi-schema/flat-union-optional-discriminator.json b/tests/qapi-schema/flat-union-optional-discriminator-no-default.json
similarity index 70%
rename from tests/qapi-schema/flat-union-optional-discriminator.json
rename to tests/qapi-schema/flat-union-optional-discriminator-no-default.json
index 08a8f7ef8b..0d612f5a62 100644
--- a/tests/qapi-schema/flat-union-optional-discriminator.json
+++ b/tests/qapi-schema/flat-union-optional-discriminator-no-default.json
@@ -1,10 +1,11 @@
-# we require the discriminator to be non-optional
+# Using an optional discriminator requires specifying a default
+# variant
 { 'enum': 'Enum', 'data': [ 'one', 'two' ] }
 { 'struct': 'Base',
   'data': { '*switch': 'Enum' } }
 { 'struct': 'Branch', 'data': { 'name': 'str' } }
 { 'union': 'MyUnion',
   'base': 'Base',
-  'discriminator': '*switch',
+  'discriminator': 'switch',
   'data': { 'one': 'Branch',
             'two': 'Branch' } }
diff --git a/tests/qapi-schema/flat-union-superfluous-default-variant.json b/tests/qapi-schema/flat-union-superfluous-default-variant.json
new file mode 100644
index 0000000000..8558165868
--- /dev/null
+++ b/tests/qapi-schema/flat-union-superfluous-default-variant.json
@@ -0,0 +1,11 @@
+# default-variant only makes sense with an optional discriminator
+{ 'enum': 'Enum', 'data': [ 'one', 'two' ] }
+{ 'struct': 'Base',
+  'data': { 'switch': 'Enum' } }
+{ 'struct': 'Branch', 'data': { 'name': 'str' } }
+{ 'union': 'MyUnion',
+  'base': 'Base',
+  'discriminator': 'switch',
+  'default-variant': 'one',
+  'data': { 'one': 'Branch',
+            'two': 'Branch' } }
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 46c7282945..64d5f6694e 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -99,6 +99,15 @@
 { 'struct': 'UserDefC',
   'data': { 'string1': 'str', 'string2': 'str' } }
 
+# for testing unions with an optional discriminator
+{ 'union': 'UserDefFlatUnion3',
+  'base': { '*enum1': 'EnumOne' },
+  'discriminator': 'enum1',
+  'default-variant': 'value1',
+  'data': { 'value1' : 'UserDefA',
+            'value2' : 'UserDefB',
+            'value3' : 'UserDefB' } }
+
 # for testing use of 'number' within alternates
 { 'alternate': 'AltEnumBool', 'data': { 'e': 'EnumOne', 'b': 'bool' } }
 { 'alternate': 'AltEnumNum', 'data': { 'e': 'EnumOne', 'n': 'number' } }
diff --git a/tests/qapi-schema/flat-union-optional-discriminator-invalid-default.err b/tests/qapi-schema/flat-union-optional-discriminator-invalid-default.err
new file mode 100644
index 0000000000..b6bd3423d6
--- /dev/null
+++ b/tests/qapi-schema/flat-union-optional-discriminator-invalid-default.err
@@ -0,0 +1 @@
+tests/qapi-schema/flat-union-optional-discriminator-invalid-default.json:7: Default variant 'three' of flat union 'MyUnion' is not part of 'Enum'
diff --git a/tests/qapi-schema/flat-union-optional-discriminator.exit b/tests/qapi-schema/flat-union-optional-discriminator-invalid-default.exit
similarity index 100%
rename from tests/qapi-schema/flat-union-optional-discriminator.exit
rename to tests/qapi-schema/flat-union-optional-discriminator-invalid-default.exit
diff --git a/tests/qapi-schema/flat-union-optional-discriminator.out b/tests/qapi-schema/flat-union-optional-discriminator-invalid-default.out
similarity index 100%
rename from tests/qapi-schema/flat-union-optional-discriminator.out
rename to tests/qapi-schema/flat-union-optional-discriminator-invalid-default.out
diff --git a/tests/qapi-schema/flat-union-optional-discriminator-invalid-specification.err b/tests/qapi-schema/flat-union-optional-discriminator-invalid-specification.err
new file mode 100644
index 0000000000..cbf154e726
--- /dev/null
+++ b/tests/qapi-schema/flat-union-optional-discriminator-invalid-specification.err
@@ -0,0 +1 @@
+tests/qapi-schema/flat-union-optional-discriminator-invalid-specification.json:7: Discriminator of flat union 'MyUnion' does not allow optional name '*switch'
diff --git a/tests/qapi-schema/flat-union-optional-discriminator-invalid-specification.exit b/tests/qapi-schema/flat-union-optional-discriminator-invalid-specification.exit
new file mode 100644
index 0000000000..d00491fd7e
--- /dev/null
+++ b/tests/qapi-schema/flat-union-optional-discriminator-invalid-specification.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/flat-union-optional-discriminator-invalid-specification.out b/tests/qapi-schema/flat-union-optional-discriminator-invalid-specification.out
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/qapi-schema/flat-union-optional-discriminator-no-default.err b/tests/qapi-schema/flat-union-optional-discriminator-no-default.err
new file mode 100644
index 0000000000..1342efd9e8
--- /dev/null
+++ b/tests/qapi-schema/flat-union-optional-discriminator-no-default.err
@@ -0,0 +1 @@
+tests/qapi-schema/flat-union-optional-discriminator-no-default.json:7: Default variant must be specified for optional discriminator 'switch'
diff --git a/tests/qapi-schema/flat-union-optional-discriminator-no-default.exit b/tests/qapi-schema/flat-union-optional-discriminator-no-default.exit
new file mode 100644
index 0000000000..d00491fd7e
--- /dev/null
+++ b/tests/qapi-schema/flat-union-optional-discriminator-no-default.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/flat-union-optional-discriminator-no-default.out b/tests/qapi-schema/flat-union-optional-discriminator-no-default.out
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/qapi-schema/flat-union-optional-discriminator.err b/tests/qapi-schema/flat-union-optional-discriminator.err
deleted file mode 100644
index aaabedb3bd..0000000000
--- a/tests/qapi-schema/flat-union-optional-discriminator.err
+++ /dev/null
@@ -1 +0,0 @@
-tests/qapi-schema/flat-union-optional-discriminator.json:6: Discriminator of flat union 'MyUnion' does not allow optional name '*switch'
diff --git a/tests/qapi-schema/flat-union-superfluous-default-variant.err b/tests/qapi-schema/flat-union-superfluous-default-variant.err
new file mode 100644
index 0000000000..5230bbf645
--- /dev/null
+++ b/tests/qapi-schema/flat-union-superfluous-default-variant.err
@@ -0,0 +1 @@
+tests/qapi-schema/flat-union-superfluous-default-variant.json:6: Must not specify a default variant for non-optional discriminator 'switch'
diff --git a/tests/qapi-schema/flat-union-superfluous-default-variant.exit b/tests/qapi-schema/flat-union-superfluous-default-variant.exit
new file mode 100644
index 0000000000..d00491fd7e
--- /dev/null
+++ b/tests/qapi-schema/flat-union-superfluous-default-variant.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/flat-union-superfluous-default-variant.out b/tests/qapi-schema/flat-union-superfluous-default-variant.out
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 542a19c407..4e3c2934bb 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -76,6 +76,15 @@ alternate UserDefAlternate
 object UserDefC
     member string1: str optional=False
     member string2: str optional=False
+object q_obj_UserDefFlatUnion3-base
+    member enum1: EnumOne optional=True
+object UserDefFlatUnion3
+    base q_obj_UserDefFlatUnion3-base
+    tag enum1
+    default variant: value1
+    case value1: UserDefA
+    case value2: UserDefB
+    case value3: UserDefB
 alternate AltEnumBool
     tag type
     case e: EnumOne
-- 
2.17.1

[Qemu-devel] [PATCH v2 04/10] qapi: Formalize qcow2 encryption probing

[Max Reitz]

Currently, you can give no encryption format for a qcow2 file while
still passing a key-secret.  That does not conform to the schema, so
this patch changes the schema to allow it.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 qapi/block-core.json | 44 ++++++++++++++++++++++++++++++++++++++++----
 block/qcow2.c        |  3 +++
 2 files changed, 43 insertions(+), 4 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 4bd85dd1bb..295ace42ae 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -44,6 +44,19 @@
 { 'struct': 'ImageInfoSpecificQCow2EncryptionBase',
   'data': { 'format': 'BlockdevQcow2EncryptionFormat'}}
 
+##
+# @ImageInfoSpecificQCow2EncryptionNoInfo:
+#
+# Only used for the qcow2 encryption format "auto" in which the actual
+# encryption format is determined from the image header.  Therefore,
+# this encryption format will never be reported in
+# ImageInfoSpecificQCow2Encryption.
+#
+# Since: 3.0
+##
+{ 'struct': 'ImageInfoSpecificQCow2EncryptionNoInfo',
+  'data': { } }
+
 ##
 # @ImageInfoSpecificQCow2Encryption:
 #
@@ -53,7 +66,8 @@
   'base': 'ImageInfoSpecificQCow2EncryptionBase',
   'discriminator': 'format',
   'data': { 'aes': 'QCryptoBlockInfoQCow',
-            'luks': 'QCryptoBlockInfoLUKS' } }
+            'luks': 'QCryptoBlockInfoLUKS',
+            'auto': 'ImageInfoSpecificQCow2EncryptionNoInfo' } }
 
 ##
 # @ImageInfoSpecificQCow2:
@@ -2658,10 +2672,30 @@
 # @BlockdevQcow2EncryptionFormat:
 # @aes: AES-CBC with plain64 initialization venctors
 #
+# @auto:            Determine the encryption format from the image
+#                   header.  This only allows the use of the
+#                   key-secret option.  (Since: 3.0)
+#
 # Since: 2.10
 ##
 { 'enum': 'BlockdevQcow2EncryptionFormat',
-  'data': [ 'aes', 'luks' ] }
+  'data': [ 'aes', 'luks', 'auto' ] }
+
+##
+# @BlockdevQcow2EncryptionSecret:
+#
+# Allows specifying a key-secret without specifying the exact
+# encryption format, which is determined automatically from the image
+# header.
+#
+# @key-secret:      The ID of a QCryptoSecret object providing the
+#                   decryption key.  Mandatory except when probing
+#                   image for metadata only.
+#
+# Since: 3.0
+##
+{ 'struct': 'BlockdevQcow2EncryptionSecret',
+  'data': { '*key-secret': 'str' } }
 
 ##
 # @BlockdevQcow2Encryption:
@@ -2669,10 +2703,12 @@
 # Since: 2.10
 ##
 { 'union': 'BlockdevQcow2Encryption',
-  'base': { 'format': 'BlockdevQcow2EncryptionFormat' },
+  'base': { '*format': 'BlockdevQcow2EncryptionFormat' },
   'discriminator': 'format',
+  'default-variant': 'auto',
   'data': { 'aes': 'QCryptoBlockOptionsQCow',
-            'luks': 'QCryptoBlockOptionsLUKS'} }
+            'luks': 'QCryptoBlockOptionsLUKS',
+            'auto': 'BlockdevQcow2EncryptionSecret' } }
 
 ##
 # @BlockdevOptionsQcow2:
diff --git a/block/qcow2.c b/block/qcow2.c
index 945132f692..6bfcba4ee1 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -868,6 +868,9 @@ static int qcow2_update_options_prepare(BlockDriverState *bs,
 
     qdict_extract_subqdict(options, &encryptopts, "encrypt.");
     encryptfmt = qdict_get_try_str(encryptopts, "format");
+    if (!g_strcmp0(encryptfmt, "auto")) {
+        encryptfmt = NULL;
+    }
 
     opts = qemu_opts_create(&qcow2_runtime_opts, NULL, 0, &error_abort);
     qemu_opts_absorb_qdict(opts, options, &local_err);
-- 
2.17.1

[Qemu-devel] [PATCH v2 05/10] qapi: Formalize qcow encryption probing

[Max Reitz]

qcow only supports a single encryption (and there is no reason why that
would change in the future), so we can make it the default.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 qapi/block-core.json | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 295ace42ae..98295ac30e 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2647,8 +2647,9 @@
 # Since: 2.10
 ##
 { 'union': 'BlockdevQcowEncryption',
-  'base': { 'format': 'BlockdevQcowEncryptionFormat' },
+  'base': { '*format': 'BlockdevQcowEncryptionFormat' },
   'discriminator': 'format',
+  'default-variant': 'aes',
   'data': { 'aes': 'QCryptoBlockOptionsQCow' } }
 
 ##
-- 
2.17.1

[Qemu-devel] [PATCH v2 06/10] qdict: Make qdict_flatten() shallow-clone-friendly

[Max Reitz]

In its current form, qdict_flatten() removes all entries from nested
QDicts that are moved to the root QDict.  It is completely sufficient to
remove all old entries from the root QDict, however.  If the nested
dicts have a refcount of 1, this will automatically delete them, too.
And if they have a greater refcount, we probably do not want to modify
them in the first place.

The latter observation means that it was currently (in general)
impossible to qdict_flatten() a shallowly cloned dict because that would
empty nested QDicts in the original dict as well.  This patch changes
this, so you can now use qdict_flatten(qdict_shallow_clone(dict)) to get
a flattened copy without disturbing the original.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 qobject/block-qdict.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/qobject/block-qdict.c b/qobject/block-qdict.c
index 0b2ae02627..7e0deb74c5 100644
--- a/qobject/block-qdict.c
+++ b/qobject/block-qdict.c
@@ -114,19 +114,30 @@ static void qdict_flatten_qdict(QDict *qdict, QDict *target, const char *prefix)
 
         /*
          * Flatten non-empty QDict and QList recursively into @target,
-         * copy other objects to @target
+         * copy other objects to @target.
+         * On the root level (if @qdict == @target), remove flattened
+         * nested QDicts and QLists from @qdict.
+         *
+         * (Note that we do not need to remove entries from nested
+         * dicts or lists.  Their reference count is decremented on
+         * the root level, so there are no leaks.  In fact, if they
+         * have a reference count greater than one, we are probably
+         * well advised not to modify them altogether.)
          */
         if (dict_val && qdict_size(dict_val)) {
             qdict_flatten_qdict(dict_val, target,
                                 new_key ? new_key : entry->key);
-            qdict_del(qdict, entry->key);
+            if (target == qdict) {
+                qdict_del(qdict, entry->key);
+            }
         } else if (list_val && !qlist_empty(list_val)) {
             qdict_flatten_qlist(list_val, target,
                                 new_key ? new_key : entry->key);
-            qdict_del(qdict, entry->key);
+            if (target == qdict) {
+                qdict_del(qdict, entry->key);
+            }
         } else if (target != qdict) {
             qdict_put_obj(target, new_key, qobject_ref(value));
-            qdict_del(qdict, entry->key);
         }
 
         g_free(new_key);
-- 
2.17.1

[Qemu-devel] [PATCH v2 07/10] tests: Add QDict clone-flatten test

[Max Reitz]

This new test verifies that qdict_flatten() does not modify a shallow
clone of the given QDict.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/check-block-qdict.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/tests/check-block-qdict.c b/tests/check-block-qdict.c
index 1d20fccbd4..478807f839 100644
--- a/tests/check-block-qdict.c
+++ b/tests/check-block-qdict.c
@@ -125,6 +125,38 @@ static void qdict_flatten_test(void)
     qobject_unref(root);
 }
 
+static void qdict_clone_flatten_test(void)
+{
+    QDict *dict1 = qdict_new();
+    QDict *dict2 = qdict_new();
+    QDict *cloned_dict1;
+
+    /*
+     * Test that we can clone and flatten
+     *    { "a": { "b": 42 } }
+     * without modifying the clone.
+     */
+
+    qdict_put_int(dict2, "b", 42);
+    qdict_put(dict1, "a", dict2);
+
+    cloned_dict1 = qdict_clone_shallow(dict1);
+
+    qdict_flatten(dict1);
+
+    g_assert(qdict_size(dict1) == 1);
+    g_assert(qdict_get_int(dict1, "a.b") == 42);
+
+    g_assert(qdict_size(cloned_dict1) == 1);
+    g_assert(qdict_get_qdict(cloned_dict1, "a") == dict2);
+
+    g_assert(qdict_size(dict2) == 1);
+    g_assert(qdict_get_int(dict2, "b") == 42);
+
+    qobject_unref(dict1);
+    qobject_unref(cloned_dict1);
+}
+
 static void qdict_array_split_test(void)
 {
     QDict *test_dict = qdict_new();
@@ -674,6 +706,7 @@ int main(int argc, char **argv)
 
     g_test_add_func("/public/defaults", qdict_defaults_test);
     g_test_add_func("/public/flatten", qdict_flatten_test);
+    g_test_add_func("/public/clone_flatten", qdict_clone_flatten_test);
     g_test_add_func("/public/array_split", qdict_array_split_test);
     g_test_add_func("/public/array_entries", qdict_array_entries_test);
     g_test_add_func("/public/join", qdict_join_test);
-- 
2.17.1

[Qemu-devel] [PATCH v2 08/10] block: Try to create well typed json:{} filenames

[Max Reitz]

By applying a health mix of qdict_flatten(), qdict_crumple(),
qdict_stringify_for_keyval(), the keyval input visitor, and the QObject
output visitor (not necessarily in that order), we can at least try to
bring bs->full_open_options into accordance with the QAPI schema.  This
may not always work (there are some options left that have not been
QAPI-fied yet), but in practice it usually will.

In any case, sometimes emitting wrongly typed json:{} filenames is
better than doing it effectively half the time.

This affects some iotests because json:{} filenames are now usually
crumpled.  In 198, "format": "auto" now appears in the qcow2 encryption
options because going through a visitor makes optional discriminators'
default values explicit.

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1534396
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c                    | 66 +++++++++++++++++++++++++++++++++++++-
 tests/qemu-iotests/059.out |  2 +-
 tests/qemu-iotests/099.out |  4 +--
 tests/qemu-iotests/110.out |  2 +-
 tests/qemu-iotests/198.out |  4 +--
 tests/qemu-iotests/207.out | 10 +++---
 6 files changed, 76 insertions(+), 12 deletions(-)

diff --git a/block.c b/block.c
index afe30caac3..6d7567b46e 100644
--- a/block.c
+++ b/block.c
@@ -36,6 +36,7 @@
 #include "qapi/qmp/qjson.h"
 #include "qapi/qmp/qnull.h"
 #include "qapi/qmp/qstring.h"
+#include "qapi/qobject-input-visitor.h"
 #include "qapi/qobject-output-visitor.h"
 #include "qapi/qapi-visit-block-core.h"
 #include "sysemu/block-backend.h"
@@ -5158,6 +5159,56 @@ static bool append_open_options(QDict *d, BlockDriverState *bs)
     return found_any;
 }
 
+/**
+ * Take a blockdev @options QDict and convert its values to the
+ * correct type.
+ *
+ * Fail if @options does not match the QAPI schema of BlockdevOptions.
+ *
+ * In case of failure, return NULL and set @errp.
+ *
+ * In case of success, return a correctly typed new QDict.
+ */
+static QDict *bdrv_type_blockdev_opts(const QDict *options, Error **errp)
+{
+    Visitor *v;
+    BlockdevOptions *blockdev_options;
+    QObject *typed_opts;
+    QDict *string_options;
+    Error *local_err = NULL;
+
+    string_options = qdict_clone_shallow(options);
+
+    qdict_flatten(string_options);
+    v = qobject_input_visitor_new_flat_confused(string_options, errp);
+    if (!v) {
+        error_prepend(errp, "Failed to prepare options: ");
+        return NULL;
+    }
+
+    visit_type_BlockdevOptions(v, NULL, &blockdev_options, &local_err);
+    visit_free(v);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        error_prepend(errp, "Not a valid BlockdevOptions object: ");
+        return NULL;
+    }
+
+    v = qobject_output_visitor_new(&typed_opts);
+    visit_type_BlockdevOptions(v, NULL, &blockdev_options, &local_err);
+    if (!local_err) {
+        visit_complete(v, &typed_opts);
+    }
+    visit_free(v);
+    qapi_free_BlockdevOptions(blockdev_options);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return NULL;
+    }
+
+    return qobject_to(QDict, typed_opts);
+}
+
 /* Updates the following BDS fields:
  *  - exact_filename: A filename which may be used for opening a block device
  *                    which (mostly) equals the given BDS (even without any
@@ -5259,10 +5310,23 @@ void bdrv_refresh_filename(BlockDriverState *bs)
     if (bs->exact_filename[0]) {
         pstrcpy(bs->filename, sizeof(bs->filename), bs->exact_filename);
     } else if (bs->full_open_options) {
-        QString *json = qobject_to_json(QOBJECT(bs->full_open_options));
+        QString *json;
+        QDict *typed_opts, *json_opts;
+
+        typed_opts = bdrv_type_blockdev_opts(bs->full_open_options, NULL);
+
+        /* We cannot be certain that bs->full_open_options matches
+         * BlockdevOptions, so bdrv_type_blockdev_opts() may fail.
+         * That is not fatal, we can just emit bs->full_open_options
+         * directly -- qemu will accept that, even if it does not
+         * match the schema. */
+        json_opts = typed_opts ?: bs->full_open_options;
+
+        json = qobject_to_json(QOBJECT(json_opts));
         snprintf(bs->filename, sizeof(bs->filename), "json:%s",
                  qstring_get_str(json));
         qobject_unref(json);
+        qobject_unref(typed_opts);
     }
 }
 
diff --git a/tests/qemu-iotests/059.out b/tests/qemu-iotests/059.out
index f6dce7947c..0238b9e9a8 100644
--- a/tests/qemu-iotests/059.out
+++ b/tests/qemu-iotests/059.out
@@ -2050,7 +2050,7 @@ wrote 512/512 bytes at offset 10240
 
 === Testing monolithicFlat with internally generated JSON file name ===
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 subformat=monolithicFlat
-can't open: Cannot use relative extent paths with VMDK descriptor file 'json:{"image": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}, "driver": "blkdebug", "inject-error.0.event": "read_aio"}'
+can't open: Cannot use relative extent paths with VMDK descriptor file 'json:{"inject-error": [{"event": "read_aio"}], "image": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}, "driver": "blkdebug"}'
 
 === Testing version 3 ===
 image: TEST_DIR/iotest-version3.IMGFMT
diff --git a/tests/qemu-iotests/099.out b/tests/qemu-iotests/099.out
index 8cce627529..0a9c434148 100644
--- a/tests/qemu-iotests/099.out
+++ b/tests/qemu-iotests/099.out
@@ -12,11 +12,11 @@ blkverify:TEST_DIR/t.IMGFMT.compare:TEST_DIR/t.IMGFMT
 
 === Testing JSON filename for blkdebug ===
 
-json:{"driver": "IMGFMT", "file": {"image": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}, "driver": "blkdebug", "inject-error.0.event": "l1_update"}}
+json:{"driver": "IMGFMT", "file": {"inject-error": [{"event": "l1_update"}], "image": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}, "driver": "blkdebug"}}
 
 === Testing indirectly enforced JSON filename ===
 
-json:{"driver": "raw", "file": {"test": {"driver": "IMGFMT", "file": {"image": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}, "driver": "blkdebug", "inject-error.0.event": "l1_update"}}, "driver": "blkverify", "raw": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.compare"}}}
+json:{"driver": "raw", "file": {"test": {"driver": "IMGFMT", "file": {"inject-error": [{"event": "l1_update"}], "image": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}, "driver": "blkdebug"}}, "driver": "blkverify", "raw": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.compare"}}}
 
 === Testing plain filename for blkdebug ===
 
diff --git a/tests/qemu-iotests/110.out b/tests/qemu-iotests/110.out
index b3584ff87f..fa19315dfb 100644
--- a/tests/qemu-iotests/110.out
+++ b/tests/qemu-iotests/110.out
@@ -11,7 +11,7 @@ backing file: t.IMGFMT.base (actual path: TEST_DIR/t.IMGFMT.base)
 
 === Non-reconstructable filename ===
 
-image: json:{"driver": "IMGFMT", "file": {"set-state.0.event": "read_aio", "image": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}, "driver": "blkdebug", "set-state.0.new_state": 42}}
+image: json:{"driver": "IMGFMT", "file": {"set-state": [{"new_state": 42, "event": "read_aio"}], "image": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}, "driver": "blkdebug"}}
 file format: IMGFMT
 virtual size: 64M (67108864 bytes)
 backing file: t.IMGFMT.base (cannot determine actual path)
diff --git a/tests/qemu-iotests/198.out b/tests/qemu-iotests/198.out
index adb805cce9..a3cb530a0c 100644
--- a/tests/qemu-iotests/198.out
+++ b/tests/qemu-iotests/198.out
@@ -32,7 +32,7 @@ read 16777216/16777216 bytes at offset 0
 16 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
 == checking image base ==
-image: json:{"encrypt.key-secret": "sec0", "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.base"}}
+image: json:{"driver": "IMGFMT", "encrypt": {"format": "auto", "key-secret": "sec0"}, "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.base"}}
 file format: IMGFMT
 virtual size: 16M (16777216 bytes)
 Format specific information:
@@ -74,7 +74,7 @@ Format specific information:
         master key iters: 1024
 
 == checking image layer ==
-image: json:{"encrypt.key-secret": "sec1", "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}}
+image: json:{"driver": "IMGFMT", "encrypt": {"format": "auto", "key-secret": "sec1"}, "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}}
 file format: IMGFMT
 virtual size: 16M (16777216 bytes)
 backing file: TEST_DIR/t.IMGFMT.base
diff --git a/tests/qemu-iotests/207.out b/tests/qemu-iotests/207.out
index 078b7e63cb..993417a77e 100644
--- a/tests/qemu-iotests/207.out
+++ b/tests/qemu-iotests/207.out
@@ -5,7 +5,7 @@
 {'execute': 'job-dismiss', 'arguments': {'id': 'job0'}}
 {u'return': {}}
 
-image: json:{"driver": "IMGFMT", "file": {"server.host": "127.0.0.1", "server.port": "22", "driver": "ssh", "path": "TEST_IMG"}}
+image: json:{"driver": "IMGFMT", "file": {"driver": "ssh", "path": "TEST_IMG", "server": {"port": "22", "host": "127.0.0.1"}}}
 file format: IMGFMT
 virtual size: 4.0M (4194304 bytes)
 
@@ -21,7 +21,7 @@ virtual size: 4.0M (4194304 bytes)
 {'execute': 'job-dismiss', 'arguments': {'id': 'job0'}}
 {u'return': {}}
 
-image: json:{"driver": "IMGFMT", "file": {"server.host": "127.0.0.1", "server.port": "22", "driver": "ssh", "path": "TEST_IMG"}}
+image: json:{"driver": "IMGFMT", "file": {"driver": "ssh", "path": "TEST_IMG", "server": {"port": "22", "host": "127.0.0.1"}}}
 file format: IMGFMT
 virtual size: 8.0M (8388608 bytes)
 
@@ -30,7 +30,7 @@ virtual size: 8.0M (8388608 bytes)
 {'execute': 'job-dismiss', 'arguments': {'id': 'job0'}}
 {u'return': {}}
 
-image: json:{"driver": "IMGFMT", "file": {"server.host": "127.0.0.1", "server.port": "22", "driver": "ssh", "path": "TEST_IMG"}}
+image: json:{"driver": "IMGFMT", "file": {"driver": "ssh", "path": "TEST_IMG", "server": {"port": "22", "host": "127.0.0.1"}}}
 file format: IMGFMT
 virtual size: 4.0M (4194304 bytes)
 
@@ -45,7 +45,7 @@ Job failed: remote host key does not match host_key_check 'wrong'
 {'execute': 'job-dismiss', 'arguments': {'id': 'job0'}}
 {u'return': {}}
 
-image: json:{"driver": "IMGFMT", "file": {"server.host": "127.0.0.1", "server.port": "22", "driver": "ssh", "path": "TEST_IMG"}}
+image: json:{"driver": "IMGFMT", "file": {"driver": "ssh", "path": "TEST_IMG", "server": {"port": "22", "host": "127.0.0.1"}}}
 file format: IMGFMT
 virtual size: 8.0M (8388608 bytes)
 
@@ -60,7 +60,7 @@ Job failed: remote host key does not match host_key_check 'wrong'
 {'execute': 'job-dismiss', 'arguments': {'id': 'job0'}}
 {u'return': {}}
 
-image: json:{"driver": "IMGFMT", "file": {"server.host": "127.0.0.1", "server.port": "22", "driver": "ssh", "path": "TEST_IMG"}}
+image: json:{"driver": "IMGFMT", "file": {"driver": "ssh", "path": "TEST_IMG", "server": {"port": "22", "host": "127.0.0.1"}}}
 file format: IMGFMT
 virtual size: 4.0M (4194304 bytes)
 
-- 
2.17.1

[Qemu-devel] [PATCH v2 09/10] iotests: Test internal option typing

[Max Reitz]

It would be nice if qemu used the correct types for blockdev options
internally, even if the user specified string values (either through
-drive or by being not so nice and using json:{} with string values).
This patch adds a test verifying that fact.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/089     | 25 +++++++++++++++++++++++++
 tests/qemu-iotests/089.out |  9 +++++++++
 2 files changed, 34 insertions(+)

diff --git a/tests/qemu-iotests/089 b/tests/qemu-iotests/089
index aa1ba4a98e..0682d08f39 100755
--- a/tests/qemu-iotests/089
+++ b/tests/qemu-iotests/089
@@ -36,6 +36,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 # get standard environment, filters and checks
 . ./common.rc
 . ./common.filter
+. ./common.qemu
 
 _supported_fmt qcow2
 _supported_proto file
@@ -145,6 +146,30 @@ $QEMU_IO -c "open -o driver=qcow2 json:{\"file.filename\":\"$TEST_IMG\"}" \
 $QEMU_IO -c "open -o driver=qcow2 json:{\"driver\":\"raw\",\"file.filename\":\"$TEST_IMG\"}" \
          -c "info" 2>&1 | _filter_img_info
 
+echo
+echo "=== Testing option typing ==="
+echo
+
+# json:{} accepts both strings and correctly typed values (even mixed,
+# although we probably do not want to support that...), but when
+# creating a json:{} filename, it should be correctly typed.
+# Therefore, both of these should make the "size" value an integer.
+
+TEST_IMG="json:{'driver': 'null-co', 'size':  42 }" _img_info | grep '^image'
+TEST_IMG="json:{'driver': 'null-co', 'size': '42'}" _img_info | grep '^image'
+
+echo
+
+# This should even work when some driver abuses bs->options to store
+# non-QAPI options (and the given -drive options are not complete)
+# (Watch for whether file.align appears as an int or a string)
+qemu_comm_method=monitor _launch_qemu \
+    -drive if=none,id=drv0,node-name=node0,format=raw,file=blkdebug::null-co://,file.align=512
+
+_send_qemu_cmd $QEMU_HANDLE 'info block' 'json:'
+
+_cleanup_qemu
+
 
 # success, all done
 echo "*** done"
diff --git a/tests/qemu-iotests/089.out b/tests/qemu-iotests/089.out
index 89e3e4340a..35ffbabe77 100644
--- a/tests/qemu-iotests/089.out
+++ b/tests/qemu-iotests/089.out
@@ -49,4 +49,13 @@ vm state offset: 512 MiB
 format name: IMGFMT
 cluster size: 64 KiB
 vm state offset: 512 MiB
+
+=== Testing option typing ===
+
+image: json:{"driver": "null-co", "size": 42}
+image: json:{"driver": "null-co", "size": 42}
+
+QEMU X.Y.Z monitor - type 'help' for more information
+(qemu) info block
+drv0 (node0): json:{"driver": "raw", "file": {"image": {"driver": "null-co"}, "driver": "blkdebug", "align": 512}} (raw)
 *** done
-- 
2.17.1

[Qemu-devel] [PATCH v2 10/10] iotests: qcow2's encrypt.format is now optional

[Max Reitz]

Remove the encrypt.format option from the two blockdev-add test cases
for encrypted qcow2 images in 087 to explicitly test that this parameter
is now optional.

Additionally, show that explicitly specifying encrypt.format=auto works
just as well, the same for specifying the correct format
(encrypt.format=luks here), and that specifying the wrong format
(encrypt.format=aes) results in an error.

While touching this test case, reduce the LUKS iteration time to 10 so
the test stays reasonably fast.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/087     | 65 +++++++++++++++++++++++---------------
 tests/qemu-iotests/087.out | 26 ++++++++++++++-
 2 files changed, 64 insertions(+), 27 deletions(-)

diff --git a/tests/qemu-iotests/087 b/tests/qemu-iotests/087
index 2561a14456..a267adf568 100755
--- a/tests/qemu-iotests/087
+++ b/tests/qemu-iotests/087
@@ -150,7 +150,6 @@ run_qemu <<EOF
           "filename": "$TEST_IMG"
       },
       "encrypt": {
-          "format": "aes",
           "key-secret": "sec0"
       }
     }
@@ -162,34 +161,48 @@ echo
 echo === Encrypted image LUKS ===
 echo
 
-_make_test_img --object secret,id=sec0,data=123456 -o encrypt.format=luks,encrypt.key-secret=sec0 $size
-run_qemu <<EOF
-{ "execute": "qmp_capabilities" }
-{ "execute": "object-add",
-  "arguments": {
-      "qom-type": "secret",
-      "id": "sec0",
-      "props": {
-          "data": "123456"
-      }
-  }
-}
-{ "execute": "blockdev-add",
-  "arguments": {
-      "driver": "$IMGFMT",
-      "node-name": "disk",
-      "file": {
-          "driver": "file",
-          "filename": "$TEST_IMG"
-      },
-      "encrypt": {
-        "format": "luks",
-        "key-secret": "sec0"
+_make_test_img \
+    --object secret,id=sec0,data=123456 \
+    -o encrypt.format=luks,encrypt.key-secret=sec0,encrypt.iter-time=10 \
+    $size
+
+# Adds the qcow2+LUKS image via blockdev-add.
+# First parameter: Optional entry for the 'encrypt' option dict
+function luks_test()
+{
+    run_qemu <<EOF
+    { "execute": "qmp_capabilities" }
+    { "execute": "object-add",
+      "arguments": {
+          "qom-type": "secret",
+          "id": "sec0",
+          "props": {
+              "data": "123456"
+          }
       }
     }
-  }
-{ "execute": "quit" }
+    { "execute": "blockdev-add",
+      "arguments": {
+          "driver": "$IMGFMT",
+          "node-name": "disk",
+          "file": {
+              "driver": "file",
+              "filename": "$TEST_IMG"
+          },
+          "encrypt": {
+            $1
+            "key-secret": "sec0"
+          }
+        }
+      }
+    { "execute": "quit" }
 EOF
+}
+
+luks_test ''                    # Implicit encrypt.format=auto
+luks_test '"format": "auto",'   # Explicit encrypt.format=auto
+luks_test '"format": "luks",'   # Explicit encrypt.format=luks
+luks_test '"format": "aes",'    # Explicit encrypt.format=aes (wrong)
 
 echo
 echo === Missing driver ===
diff --git a/tests/qemu-iotests/087.out b/tests/qemu-iotests/087.out
index b1318c6ed6..40b0a96ba7 100644
--- a/tests/qemu-iotests/087.out
+++ b/tests/qemu-iotests/087.out
@@ -46,7 +46,7 @@ QMP_VERSION
 
 === Encrypted image LUKS ===
 
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 encrypt.format=luks encrypt.key-secret=sec0
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 encrypt.format=luks encrypt.key-secret=sec0 encrypt.iter-time=10
 Testing:
 QMP_VERSION
 {"return": {}}
@@ -55,6 +55,30 @@ QMP_VERSION
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}}
 
+Testing:
+QMP_VERSION
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}}
+
+Testing:
+QMP_VERSION
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}}
+
+Testing:
+QMP_VERSION
+{"return": {}}
+{"return": {}}
+{"error": {"class": "GenericError", "desc": "Header reported 'luks' encryption format but options specify 'aes'"}}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}}
+
 
 === Missing driver ===
 
-- 
2.17.1

Re: [Qemu-devel] [PATCH v2 04/10] qapi: Formalize qcow2 encryption probing

[Eric Blake]

On 06/11/2018 03:51 PM, Max Reitz wrote:
> Currently, you can give no encryption format for a qcow2 file while
> still passing a key-secret.  That does not conform to the schema, so
> this patch changes the schema to allow it.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   qapi/block-core.json | 44 ++++++++++++++++++++++++++++++++++++++++----
>   block/qcow2.c        |  3 +++
>   2 files changed, 43 insertions(+), 4 deletions(-)

> +##
> +# @ImageInfoSpecificQCow2EncryptionNoInfo:
> +#
> +# Only used for the qcow2 encryption format "auto" in which the actual
> +# encryption format is determined from the image header.  Therefore,
> +# this encryption format will never be reported in
> +# ImageInfoSpecificQCow2Encryption.
> +#
> +# Since: 3.0
> +##
> +{ 'struct': 'ImageInfoSpecificQCow2EncryptionNoInfo',
> +  'data': { } }

Do we actually need this type, given Anton's work on making omitted 
branches automatically use an empty struct?

https://lists.gnu.org/archive/html/qemu-devel/2018-05/msg06836.html

> +
>   ##
>   # @ImageInfoSpecificQCow2Encryption:
>   #
> @@ -53,7 +66,8 @@
>     'base': 'ImageInfoSpecificQCow2EncryptionBase',
>     'discriminator': 'format',
>     'data': { 'aes': 'QCryptoBlockInfoQCow',
> -            'luks': 'QCryptoBlockInfoLUKS' } }
> +            'luks': 'QCryptoBlockInfoLUKS',
> +            'auto': 'ImageInfoSpecificQCow2EncryptionNoInfo' } }

If Anton's patches go in first, you don't even have to change this type;

>   
>   ##
>   # @ImageInfoSpecificQCow2:
> @@ -2658,10 +2672,30 @@
>   # @BlockdevQcow2EncryptionFormat:
>   # @aes: AES-CBC with plain64 initialization venctors
>   #
> +# @auto:            Determine the encryption format from the image
> +#                   header.  This only allows the use of the
> +#                   key-secret option.  (Since: 3.0)
> +#
>   # Since: 2.10
>   ##
>   { 'enum': 'BlockdevQcow2EncryptionFormat',
> -  'data': [ 'aes', 'luks' ] }
> +  'data': [ 'aes', 'luks', 'auto' ] }

the changed enum would be sufficient.

> +
> +##
> +# @BlockdevQcow2EncryptionSecret:
> +#
> +# Allows specifying a key-secret without specifying the exact
> +# encryption format, which is determined automatically from the image
> +# header.
> +#
> +# @key-secret:      The ID of a QCryptoSecret object providing the
> +#                   decryption key.  Mandatory except when probing
> +#                   image for metadata only.
> +#
> +# Since: 3.0
> +##
> +{ 'struct': 'BlockdevQcow2EncryptionSecret',
> +  'data': { '*key-secret': 'str' } }
>   
>   ##
>   # @BlockdevQcow2Encryption:
> @@ -2669,10 +2703,12 @@
>   # Since: 2.10
>   ##
>   { 'union': 'BlockdevQcow2Encryption',
> -  'base': { 'format': 'BlockdevQcow2EncryptionFormat' },
> +  'base': { '*format': 'BlockdevQcow2EncryptionFormat' },
>     'discriminator': 'format',
> +  'default-variant': 'auto',
>     'data': { 'aes': 'QCryptoBlockOptionsQCow',
> -            'luks': 'QCryptoBlockOptionsLUKS'} }
> +            'luks': 'QCryptoBlockOptionsLUKS',
> +            'auto': 'BlockdevQcow2EncryptionSecret' } }

This part is necessary, though, and looks correct.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v2 05/10] qapi: Formalize qcow encryption probing

[Eric Blake]

On 06/11/2018 03:51 PM, Max Reitz wrote:
> qcow only supports a single encryption (and there is no reason why that
> would change in the future), so we can make it the default.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   qapi/block-core.json | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 295ace42ae..98295ac30e 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2647,8 +2647,9 @@
>   # Since: 2.10
>   ##
>   { 'union': 'BlockdevQcowEncryption',
> -  'base': { 'format': 'BlockdevQcowEncryptionFormat' },
> +  'base': { '*format': 'BlockdevQcowEncryptionFormat' },
>     'discriminator': 'format',
> +  'default-variant': 'aes',
>     'data': { 'aes': 'QCryptoBlockOptionsQCow' } }
>   
>   ##
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v2 04/10] qapi: Formalize qcow2 encryption probing

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1430 bytes --]

On 2018-06-11 23:02, Eric Blake wrote:
> On 06/11/2018 03:51 PM, Max Reitz wrote:
>> Currently, you can give no encryption format for a qcow2 file while
>> still passing a key-secret.  That does not conform to the schema, so
>> this patch changes the schema to allow it.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   qapi/block-core.json | 44 ++++++++++++++++++++++++++++++++++++++++----
>>   block/qcow2.c        |  3 +++
>>   2 files changed, 43 insertions(+), 4 deletions(-)
> 
>> +##
>> +# @ImageInfoSpecificQCow2EncryptionNoInfo:
>> +#
>> +# Only used for the qcow2 encryption format "auto" in which the actual
>> +# encryption format is determined from the image header.  Therefore,
>> +# this encryption format will never be reported in
>> +# ImageInfoSpecificQCow2Encryption.
>> +#
>> +# Since: 3.0
>> +##
>> +{ 'struct': 'ImageInfoSpecificQCow2EncryptionNoInfo',
>> +  'data': { } }
> 
> Do we actually need this type, given Anton's work on making omitted
> branches automatically use an empty struct?
> 
> https://lists.gnu.org/archive/html/qemu-devel/2018-05/msg06836.html

Looks like no, we don't.  Great! :-)

I think I'll still keep part of the comment and move it down into the
description of ImageInfoSpecificQCow2Encryption so that anyone who's
wondering knows that this value won't appear.

Thanks for reviewing and pointing me at it,

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v2 04/10] qapi: Formalize qcow2 encryption probing

[Daniel P. Berrangé]

On Mon, Jun 11, 2018 at 10:51:57PM +0200, Max Reitz wrote:
> Currently, you can give no encryption format for a qcow2 file while
> still passing a key-secret.  That does not conform to the schema, so
> this patch changes the schema to allow it.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  qapi/block-core.json | 44 ++++++++++++++++++++++++++++++++++++++++----
>  block/qcow2.c        |  3 +++
>  2 files changed, 43 insertions(+), 4 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v2 05/10] qapi: Formalize qcow encryption probing

[Daniel P. Berrangé]

On Mon, Jun 11, 2018 at 10:51:58PM +0200, Max Reitz wrote:
> qcow only supports a single encryption (and there is no reason why that
> would change in the future), so we can make it the default.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  qapi/block-core.json | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v2 10/10] iotests: qcow2's encrypt.format is now optional

[Daniel P. Berrangé]

On Mon, Jun 11, 2018 at 10:52:03PM +0200, Max Reitz wrote:
> Remove the encrypt.format option from the two blockdev-add test cases
> for encrypted qcow2 images in 087 to explicitly test that this parameter
> is now optional.
> 
> Additionally, show that explicitly specifying encrypt.format=auto works
> just as well, the same for specifying the correct format
> (encrypt.format=luks here), and that specifying the wrong format
> (encrypt.format=aes) results in an error.
> 
> While touching this test case, reduce the LUKS iteration time to 10 so
> the test stays reasonably fast.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/087     | 65 +++++++++++++++++++++++---------------
>  tests/qemu-iotests/087.out | 26 ++++++++++++++-
>  2 files changed, 64 insertions(+), 27 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v2 06/10] qdict: Make qdict_flatten() shallow-clone-friendly

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> In its current form, qdict_flatten() removes all entries from nested
> QDicts that are moved to the root QDict.  It is completely sufficient to
> remove all old entries from the root QDict, however.  If the nested
> dicts have a refcount of 1, this will automatically delete them, too.
> And if they have a greater refcount, we probably do not want to modify
> them in the first place.
>
> The latter observation means that it was currently (in general)
> impossible to qdict_flatten() a shallowly cloned dict because that would
> empty nested QDicts in the original dict as well.  This patch changes
> this, so you can now use qdict_flatten(qdict_shallow_clone(dict)) to get
> a flattened copy without disturbing the original.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  qobject/block-qdict.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/qobject/block-qdict.c b/qobject/block-qdict.c
> index 0b2ae02627..7e0deb74c5 100644
> --- a/qobject/block-qdict.c
> +++ b/qobject/block-qdict.c
> @@ -114,19 +114,30 @@ static void qdict_flatten_qdict(QDict *qdict, QDict *target, const char *prefix)
>  
>          /*
>           * Flatten non-empty QDict and QList recursively into @target,
> -         * copy other objects to @target
> +         * copy other objects to @target.
> +         * On the root level (if @qdict == @target), remove flattened
> +         * nested QDicts and QLists from @qdict.
> +         *
> +         * (Note that we do not need to remove entries from nested
> +         * dicts or lists.  Their reference count is decremented on
> +         * the root level, so there are no leaks.  In fact, if they
> +         * have a reference count greater than one, we are probably
> +         * well advised not to modify them altogether.)
>           */
>          if (dict_val && qdict_size(dict_val)) {
>              qdict_flatten_qdict(dict_val, target,
>                                  new_key ? new_key : entry->key);
> -            qdict_del(qdict, entry->key);
> +            if (target == qdict) {
> +                qdict_del(qdict, entry->key);
> +            }
>          } else if (list_val && !qlist_empty(list_val)) {
>              qdict_flatten_qlist(list_val, target,
>                                  new_key ? new_key : entry->key);
> -            qdict_del(qdict, entry->key);
> +            if (target == qdict) {
> +                qdict_del(qdict, entry->key);
> +            }
>          } else if (target != qdict) {
>              qdict_put_obj(target, new_key, qobject_ref(value));
> -            qdict_del(qdict, entry->key);
>          }
>  
>          g_free(new_key);

This simply makes sense, even without the later patch(es) that rely on
it.

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

Re: [Qemu-devel] [PATCH v2 07/10] tests: Add QDict clone-flatten test

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> This new test verifies that qdict_flatten() does not modify a shallow
> clone of the given QDict.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Max Reitz <mreitz@redhat.com>

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

Re: [Qemu-devel] [PATCH v2 01/10] qapi: Add default-variant for flat unions

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> This patch allows specifying a discriminator that is an optional member
> of the base struct.  In such a case, a default value must be provided
> that is used when no value is given.

Hmm.  Can you explain why you need this feature?

> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  qapi/introspect.json           |  8 +++++
>  scripts/qapi/common.py         | 57 ++++++++++++++++++++++++++++------
>  scripts/qapi/doc.py            |  8 +++--
>  scripts/qapi/introspect.py     | 10 ++++--
>  scripts/qapi/visit.py          | 13 ++++++++
>  tests/qapi-schema/test-qapi.py |  2 ++
>  6 files changed, 83 insertions(+), 15 deletions(-)

The documentation update is in the next patch, and tests are in the
patch after next.  I'd squash all three.

>
> diff --git a/qapi/introspect.json b/qapi/introspect.json
> index 80a0a3e656..b43c87fe8d 100644
> --- a/qapi/introspect.json
> +++ b/qapi/introspect.json
> @@ -168,6 +168,13 @@
>  # @tag: the name of the member serving as type tag.
>  #       An element of @members with this name must exist.
>  #
> +# @default-variant: if the @tag element of @members is optional, this
> +#                   is the default value for choosing a variant.  Its
> +#                   value will be a valid value for @tag.
> +#                   Present exactly when @tag is present and the
> +#                   associated element of @members is optional.
> +#                   (Since: 3.0)
> +#
>  # @variants: variant members, i.e. additional members that
>  #            depend on the type tag's value.  Present exactly when
>  #            @tag is present.  The variants are in no particular order,
> @@ -181,6 +188,7 @@
>  { 'struct': 'SchemaInfoObject',
>    'data': { 'members': [ 'SchemaInfoObjectMember' ],
>              '*tag': 'str',
> +            '*default-variant': 'str',
>              '*variants': [ 'SchemaInfoObjectVariant' ] } }
>  
>  ##

Until now, the QAPI schema doesn't let you specify default values.
Instead, code sees "absent", and does whatever needs to be done.
Sometimes, it supplies a default value.  "Absent" is shorthand for this
value then.  Sometimes, it behaves differently than for any value.
"Absent" is a distinct additional case then.  I'm not too fond of the
latter case, but it's what we have, and it's not going away.

We've discussed extending the QAPI schema to let you specify default
values.  Two advantages: the default value is *specified* rather than
just documented (or not, when documentation sucks), and we can supply
the default value in generated code.  The introspection schema is even
prepared for this:

    ##
    # @SchemaInfoObjectMember:
    #
    # An object member.
    #
    # @name: the member's name, as defined in the QAPI schema.
    #
    # @type: the name of the member's type.
    #
--> # @default: default when used as command parameter.
    #           If absent, the parameter is mandatory.
    #           If present, the value must be null.  The parameter is
    #           optional, and behavior when it's missing is not specified
    #           here.
    #           Future extension: if present and non-null, the parameter
    #           is optional, and defaults to this value.
    #
    # Since: 2.5
    ##

The idea was to turn

    '*name': 'type'

into sugar for

     'name': { 'type': 'type', 'optional': true }

then extend it to

     'name': { 'type': 'type', 'optional': true, 'default': JSON-VALUE }

where JSON-VALUE null means behavior for "absent" is not specified.

Your patch adds default values in a different way, and for just for
union tags.  For the QAPI language, that's not necessarily a deal
breaker, as we don't need to maintain backward compatibility there.  But
for the introspection schema, we do.  Once we add @default-variant,
we're stuck with it.  Even though it's redundant with
SchemaInfoObjectMember's @default.

I'm skipping review of the implementation for now.

Re: [Qemu-devel] [PATCH v2 01/10] qapi: Add default-variant for flat unions

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 7335 bytes --]

On 2018-06-12 17:25, Markus Armbruster wrote:
> Max Reitz <mreitz@redhat.com> writes:
> 
>> This patch allows specifying a discriminator that is an optional member
>> of the base struct.  In such a case, a default value must be provided
>> that is used when no value is given.
> 
> Hmm.  Can you explain why you need this feature?

Define "need".

Most block drivers support exactly the same syntax with -drive and with
-blockdev.  Type confusion occurs, but at least the options are the same.

One notable example is rbd's =keyvalue-pairs, but we all probably agree
on the fact that it's just broken and we don't have to care too much.

The other case I noticed (and so far the only other case I know of
besides =keyvalue-pairs) is qcow(2) encryption.  The qcow(2) driver
allows you to specify different parameters under @encrypt depending on
the encryption format, so that naturally becomes a flat union with
encrypt.format being the discriminator.

Now when opening a qcow(2) file, the header already gives you the
encryption format (always qcow AES for qcow v1, qcow AES or LUKS for
qcow2), so the user technically doesn't need to specify it.  And that is
what the qcow(2) driver allows on the command line: The user does not
need to specify the format as it can be inferred from the header, and
the union is interpreted accordingly.  Of course, that doesn't fly with
-blockdev.

Now luckily both AES and LUKS only allow you to specify a key-secret
option.  Therefore, with an optional discriminator, we can define a
default variant that just has this key-secret option.  This is what this
patch is needed for.  It allows full compatibility to the current
interface, although it means we'll have to watch out when adding
encryption options in the future.

The alternative I saw was to decide that auto-detection was a bad idea
and to make encrypt.format mandatory for -drive.  Maybe I would have
taken that approach had I known how little I know about the QAPI
generator at this point.  But after I'd started, it was a challenge, so
I continued down that other path.  Anyway, the problem with this
approach would be obviously compatibility breakage, and also being a bit
cumbersome.  ("Why would you need to specify encrypt.format when it's
all in the image header anyway?")


Short answer: The current QAPI schema for qcow2 is incompatible to what
it allows with -drive, this patch allows making it compatible -- and we
need both to be compatible if we ever want BlockdevOptions throughout
the block layer.

>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  qapi/introspect.json           |  8 +++++
>>  scripts/qapi/common.py         | 57 ++++++++++++++++++++++++++++------
>>  scripts/qapi/doc.py            |  8 +++--
>>  scripts/qapi/introspect.py     | 10 ++++--
>>  scripts/qapi/visit.py          | 13 ++++++++
>>  tests/qapi-schema/test-qapi.py |  2 ++
>>  6 files changed, 83 insertions(+), 15 deletions(-)
> 
> The documentation update is in the next patch, and tests are in the
> patch after next.  I'd squash all three.

I don't mind.

(I just like to split patches in advance, because squashing is always easy.)

>> diff --git a/qapi/introspect.json b/qapi/introspect.json
>> index 80a0a3e656..b43c87fe8d 100644
>> --- a/qapi/introspect.json
>> +++ b/qapi/introspect.json
>> @@ -168,6 +168,13 @@
>>  # @tag: the name of the member serving as type tag.
>>  #       An element of @members with this name must exist.
>>  #
>> +# @default-variant: if the @tag element of @members is optional, this
>> +#                   is the default value for choosing a variant.  Its
>> +#                   value will be a valid value for @tag.
>> +#                   Present exactly when @tag is present and the
>> +#                   associated element of @members is optional.
>> +#                   (Since: 3.0)
>> +#
>>  # @variants: variant members, i.e. additional members that
>>  #            depend on the type tag's value.  Present exactly when
>>  #            @tag is present.  The variants are in no particular order,
>> @@ -181,6 +188,7 @@
>>  { 'struct': 'SchemaInfoObject',
>>    'data': { 'members': [ 'SchemaInfoObjectMember' ],
>>              '*tag': 'str',
>> +            '*default-variant': 'str',
>>              '*variants': [ 'SchemaInfoObjectVariant' ] } }
>>  
>>  ##
> 
> Until now, the QAPI schema doesn't let you specify default values.
> Instead, code sees "absent", and does whatever needs to be done.
> Sometimes, it supplies a default value.  "Absent" is shorthand for this
> value then.  Sometimes, it behaves differently than for any value.
> "Absent" is a distinct additional case then.  I'm not too fond of the
> latter case, but it's what we have, and it's not going away.

Just a note: In v1 of this series, it was possible to detect when the
variant was indeed absent.  Eric proposed just setting the member to the
default value when absent (because he too is not too fond of that case),
so here in v2 there is no difference between setting the value to its
default value or not specifying it.

> We've discussed extending the QAPI schema to let you specify default
> values.  Two advantages: the default value is *specified* rather than
> just documented (or not, when documentation sucks), and we can supply
> the default value in generated code.  The introspection schema is even
> prepared for this:
> 
>     ##
>     # @SchemaInfoObjectMember:
>     #
>     # An object member.
>     #
>     # @name: the member's name, as defined in the QAPI schema.
>     #
>     # @type: the name of the member's type.
>     #
> --> # @default: default when used as command parameter.
>     #           If absent, the parameter is mandatory.
>     #           If present, the value must be null.  The parameter is
>     #           optional, and behavior when it's missing is not specified
>     #           here.
>     #           Future extension: if present and non-null, the parameter
>     #           is optional, and defaults to this value.
>     #
>     # Since: 2.5
>     ##
> 
> The idea was to turn
> 
>     '*name': 'type'
> 
> into sugar for
> 
>      'name': { 'type': 'type', 'optional': true }
> 
> then extend it to
> 
>      'name': { 'type': 'type', 'optional': true, 'default': JSON-VALUE }
> 
> where JSON-VALUE null means behavior for "absent" is not specified.

Seems reasonable.

> Your patch adds default values in a different way, and for just for
> union tags.  For the QAPI language, that's not necessarily a deal
> breaker, as we don't need to maintain backward compatibility there.  But
> for the introspection schema, we do.  Once we add @default-variant,
> we're stuck with it.  Even though it's redundant with
> SchemaInfoObjectMember's @default.

I too would like a general solution better.  Of course the question is
how far off such a general solution is.  It's not like I consider this
series extremely urgent, but I do have a BZ assigned for it, so take
that as you will. O:-)

So between the lines I'd like to think I can read that it would be
"fine" to add default-variant for now, but to omit it from introspection?

Max

> I'm skipping review of the implementation for now


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v2 00/10] block: Try to create well typed json:{} filenames

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> See cover letter of v1 here:
> http://lists.nongnu.org/archive/html/qemu-block/2018-05/msg00290.html
>
>
> This series depends on Markus's series "block: Configuration fixes and
> rbd authentication":
>
> Based-on: <20180607062559.16127-1-armbru@redhat.com>

PATCH 06+07 applied to qapi-next as fix of latent bug.  Thanks!