All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v3 0/8] block: Try to create well-typed json:{} filenames
@ 2019-02-06 19:55 Max Reitz
  2019-02-06 19:55 ` [Qemu-devel] [PATCH v3 1/8] qapi: Add default-variant for flat unions Max Reitz
                   ` (7 more replies)
  0 siblings, 8 replies; 22+ messages in thread
From: Max Reitz @ 2019-02-06 19:55 UTC (permalink / raw)
  To: qemu-block
  Cc: qemu-devel, Max Reitz, Eric Blake, Markus Armbruster,
	Michael Roth, Kevin Wolf

This series depends on my "Fix some filename generation issues" series.

Based-on: <20190201192935.18394-1-mreitz@redhat.com>


When qemu reports json:{} filename, it just uses whatever type you gave
an option in.  With -drive, all options are strings and they do not have
to pass the test of the typing firewall of the QAPI schema, so you just
get strings thrown back at you even if that does not match the schema.
(Also, if you use json:{} yourself, you’re free to give the options as
strings as well.)

Example:

$ ./qemu-img info --image-opts driver=raw,size=512,file.driver=null-co
image: json:{"driver": "raw", "size": "512", "file": {"driver": "null-co"}}

@size is supposed to be an integer, according to the schema, so the
correct result would be (which is what you get after this series):

$ ./qemu-img info --image-opts driver=raw,size=512,file.driver=null-co
image: json:{"driver": "raw", "size": 512, "file": {"driver": "null-co"}}


This is achieved by patch 6, which makes bdrv_refresh_filename() run the
options through the flat-confused input visitor, and then through the
output visitor to get all to the correct type.  If anything fails, the
result is as before (hence the “Try” in the title).

There are cases where this cannot work.  Those are the ones where -drive
accepts something that is not allowed by the QAPI schema.  One of these
cases is rbd’s =keyvalue-pairs, which is just broken altogether, so
let’s simply ignore that.  (I don’t think anybody’s going to complain
that the json:{} filename they get is not correctly typed after they’ve
used that option.)

The other case (I know of) is qcow(2)’s encryption.  In the QAPI schema,
encrypt.format is not optional because it is the discriminator for which
kind of options to use.  However, for -drive, it is optional because the
qcow2 driver can infer the encryption format from the image header.

The solution that’s proposed by this series is to make flat union
discriminators optional and provide a default.  Both AES and LUKS
encryption allow only a key-secret option, so we can add a new
pseudo-format “auto” that accepts exactly that option and makes the
qcow2 driver read the real format from the image header.  This
pseudo-format is made the default for encrypt.format, and thus you can
then specify encrypt.key-secret without having to specify
encrypt.format (while still adhering to the QAPI schema).


So, in this series:
- The QAPI code generator is modified to allow optional discriminators
  for flat unions.  In such a case, a default-variant must be supplied
  to choose when the discriminator value is not present on the wire.
  - Accordingly, documentation, tests, and introspection are adjusted.

- This is used to make qcow’s and qcow2’s encrypt.format parameter
  optional.  It now defaults to “from-image” which is a new
  pseudo-format that allows a key-secret to be given, and otherwise
  leaves it to the format driver to determine the encryption format.

- json:{} filenames are attempted to be typed correctly when they are
  generated, by running bs->full_open_options through a healthy mix of
  qdict_flatten(), the flat-confused input visitor for BlockdevOptions,
  and the output visitor.
  This may not always work but I hope it usually will.  Fingers crossed.
  (At least it won’t make things worse.)

- Tests, tests, tests.


(Yes, I know that “In this series tests, tests, tests.” is not a
 sentence.)


v3:
- Rebased
- Dropped the new superfluous empty struct
  ImageInfoSpecificQCow2EncryptionNoInfo, and moved some of its
  description to ImageInfoSpecificQCow2Encryption


git-backport-diff against v2:

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/8:[0037] [FC] 'qapi: Add default-variant for flat unions'
002/8:[----] [-C] 'docs/qapi: Document optional discriminators'
003/8:[0005] [FC] 'tests: Add QAPI optional discriminator tests'
004/8:[0021] [FC] 'qapi: Formalize qcow2 encryption probing'
005/8:[----] [--] 'qapi: Formalize qcow encryption probing'
006/8:[0006] [FC] 'block: Try to create well typed json:{} filenames'
007/8:[----] [--] 'iotests: Test internal option typing'
008/8:[0006] [FC] 'iotests: qcow2's encrypt.format is now optional'


Max Reitz (8):
  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
  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                          | 34 ++++++++--
 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       | 11 +++
 block.c                                       | 68 ++++++++++++++++++-
 block/qcow2.c                                 |  3 +
 scripts/qapi/common.py                        | 58 +++++++++++++---
 scripts/qapi/doc.py                           | 10 ++-
 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        | 10 +++
 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 +--
 39 files changed, 384 insertions(+), 64 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.20.1

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [Qemu-devel] [PATCH v3 1/8] qapi: Add default-variant for flat unions
  2019-02-06 19:55 [Qemu-devel] [PATCH v3 0/8] block: Try to create well-typed json:{} filenames Max Reitz
@ 2019-02-06 19:55 ` Max Reitz
  2019-02-06 20:13   ` Eric Blake
  2019-02-07  6:56   ` Markus Armbruster
  2019-02-06 19:55 ` [Qemu-devel] [PATCH v3 2/8] docs/qapi: Document optional discriminators Max Reitz
                   ` (6 subsequent siblings)
  7 siblings, 2 replies; 22+ messages in thread
From: Max Reitz @ 2019-02-06 19:55 UTC (permalink / raw)
  To: qemu-block
  Cc: qemu-devel, Max Reitz, Eric Blake, Markus Armbruster,
	Michael Roth, Kevin Wolf

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         | 58 ++++++++++++++++++++++++++++------
 scripts/qapi/doc.py            | 10 ++++--
 scripts/qapi/introspect.py     | 10 ++++--
 scripts/qapi/visit.py          | 13 ++++++++
 tests/qapi-schema/test-qapi.py |  2 ++
 6 files changed, 86 insertions(+), 15 deletions(-)

diff --git a/qapi/introspect.json b/qapi/introspect.json
index 3d22166b2b..e4740aa7a0 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: 4.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 c89edc0cb0..13fd55d9c9 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -750,6 +750,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.
@@ -774,16 +775,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_value = base_members.get(discriminator)
-        if not discriminator_value:
-            raise QAPISemError(info,
-                               "Discriminator '%s' is not a member of base "
-                               "struct '%s'"
-                               % (discriminator, base))
+        if default_variant is None:
+            discriminator_value = base_members.get(discriminator)
+            if not discriminator_value:
+                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_value = base_members.get('*' + discriminator)
+            if not discriminator_value:
+                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)
+
         if discriminator_value.get('if'):
             raise QAPISemError(info, 'The discriminator %s.%s for union %s '
                                'must not be conditional' %
@@ -796,6 +818,16 @@ 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 not any(value for value in enum_define['data']
+                       if value['name'] == default_variant):
+                raise QAPISemError(info,
+                                   "Default variant '%s' of flat union '%s' is "
+                                   "not part of '%s'"
+                                   % (default_variant, name,
+                                      discriminator_value['type']))
+
     # Check every branch; don't allow an empty union
     if len(members) == 0:
         raise QAPISemError(info, "Union '%s' cannot have empty 'data'" % name)
@@ -976,7 +1008,7 @@ def check_exprs(exprs):
         elif 'union' in expr:
             meta = 'union'
             check_keys(expr_elem, 'union', ['data'],
-                       ['base', 'discriminator', 'if'])
+                       ['base', 'discriminator', 'if', 'default-variant'])
             normalize_members(expr.get('base'))
             normalize_members(expr['data'])
             union_types[expr[meta]] = expr
@@ -1431,12 +1463,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
@@ -1444,6 +1478,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):
@@ -1773,6 +1808,7 @@ class QAPISchema(object):
         base = expr.get('base')
         ifcond = expr.get('if')
         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(
@@ -1794,6 +1830,7 @@ class QAPISchema(object):
             QAPISchemaObjectType(name, info, doc, ifcond, base, members,
                                  QAPISchemaObjectTypeVariants(tag_name,
                                                               tag_member,
+                                                              default_tag_value,
                                                               variants)))
 
     def _def_alternate_type(self, expr, info, doc):
@@ -1807,6 +1844,7 @@ class QAPISchema(object):
             QAPISchemaAlternateType(name, info, doc, ifcond,
                                     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 c03b690161..c7ad23985b 100755
--- a/scripts/qapi/doc.py
+++ b/scripts/qapi/doc.py
@@ -168,8 +168,14 @@ 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"}%s' % (
-                variants.tag_member.name, v.name, texi_if(v.ifcond, " (", ")"))
+            if v.name == variants.default_tag_value:
+                when = ' when @code{%s} is @t{"%s"} or not given%s' % (
+                    variants.tag_member.name, v.name,
+                    texi_if(v.ifcond, " (", ")"))
+            else:
+                when = ' when @code{%s} is @t{"%s"}%s' % (
+                    variants.tag_member.name, v.name,
+                    texi_if(v.ifcond, " (", ")"))
             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 f7f2ca07e4..db8af348d2 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -166,9 +166,12 @@ const QLitObject %(c_name)s = %(c_string)s;
             ret = (ret, {'if': member.ifcond})
         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)},
@@ -192,6 +195,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, ifcond)
 
diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
index 82eab72b21..57eccdfdc2 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -77,6 +77,19 @@ void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp)
         ret += gen_endif(memb.ifcond)
 
     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 d592854601..be223e2cc4 100644
--- a/tests/qapi-schema/test-qapi.py
+++ b/tests/qapi-schema/test-qapi.py
@@ -66,6 +66,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))
                 QAPISchemaTestVisitor._print_if(v.ifcond, indent=8)
-- 
2.20.1

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [Qemu-devel] [PATCH v3 2/8] docs/qapi: Document optional discriminators
  2019-02-06 19:55 [Qemu-devel] [PATCH v3 0/8] block: Try to create well-typed json:{} filenames Max Reitz
  2019-02-06 19:55 ` [Qemu-devel] [PATCH v3 1/8] qapi: Add default-variant for flat unions Max Reitz
@ 2019-02-06 19:55 ` Max Reitz
  2019-02-06 19:55 ` [Qemu-devel] [PATCH v3 3/8] tests: Add QAPI optional discriminator tests Max Reitz
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 22+ messages in thread
From: Max Reitz @ 2019-02-06 19:55 UTC (permalink / raw)
  To: qemu-block
  Cc: qemu-devel, Max Reitz, Eric Blake, Markus Armbruster,
	Michael Roth, Kevin Wolf

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 87183d3a09..32c576f30b 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
@@ -504,6 +504,23 @@ 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.20.1

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [Qemu-devel] [PATCH v3 3/8] tests: Add QAPI optional discriminator tests
  2019-02-06 19:55 [Qemu-devel] [PATCH v3 0/8] block: Try to create well-typed json:{} filenames Max Reitz
  2019-02-06 19:55 ` [Qemu-devel] [PATCH v3 1/8] qapi: Add default-variant for flat unions Max Reitz
  2019-02-06 19:55 ` [Qemu-devel] [PATCH v3 2/8] docs/qapi: Document optional discriminators Max Reitz
@ 2019-02-06 19:55 ` Max Reitz
  2019-02-06 20:20   ` Eric Blake
  2019-02-06 19:55 ` [Qemu-devel] [PATCH v3 4/8] qapi: Formalize qcow2 encryption probing Max Reitz
                   ` (4 subsequent siblings)
  7 siblings, 1 reply; 22+ messages in thread
From: Max Reitz @ 2019-02-06 19:55 UTC (permalink / raw)
  To: qemu-block
  Cc: qemu-devel, Max Reitz, Eric Blake, Markus Armbruster,
	Michael Roth, Kevin Wolf

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              | 11 +++++++++++
 ...-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               | 10 ++++++++++
 20 files changed, 70 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 75ad9c0dd3..8fd05da00a 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -422,8 +422,11 @@ qapi-schema += flat-union-invalid-branch-key.json
 qapi-schema += flat-union-invalid-discriminator.json
 qapi-schema += flat-union-invalid-if-discriminator.json
 qapi-schema += flat-union-no-base.json
-qapi-schema += flat-union-optional-discriminator.json
+qapi-schema += flat-union-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 cb0857df52..6c7462928e 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -104,6 +104,17 @@
 { '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'
+            # 'value4' defaults to empty
+  } }
+
 # 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 9464101d26..0044fb4554 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -92,6 +92,16 @@ 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
+    case value4: q_empty
 alternate AltEnumBool
     tag type
     case e: EnumOne
-- 
2.20.1

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [Qemu-devel] [PATCH v3 4/8] qapi: Formalize qcow2 encryption probing
  2019-02-06 19:55 [Qemu-devel] [PATCH v3 0/8] block: Try to create well-typed json:{} filenames Max Reitz
                   ` (2 preceding siblings ...)
  2019-02-06 19:55 ` [Qemu-devel] [PATCH v3 3/8] tests: Add QAPI optional discriminator tests Max Reitz
@ 2019-02-06 19:55 ` Max Reitz
  2019-02-06 20:23   ` Eric Blake
  2019-02-06 19:55 ` [Qemu-devel] [PATCH v3 5/8] qapi: Formalize qcow " Max Reitz
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 22+ messages in thread
From: Max Reitz @ 2019-02-06 19:55 UTC (permalink / raw)
  To: qemu-block
  Cc: qemu-devel, Max Reitz, Eric Blake, Markus Armbruster,
	Michael Roth, Kevin Wolf

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 | 31 ++++++++++++++++++++++++++++---
 block/qcow2.c        |  3 +++
 2 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 5f17d67d71..abf9af0c4d 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -47,6 +47,9 @@
 ##
 # @ImageInfoSpecificQCow2Encryption:
 #
+# @format will never be "auto", as this pseudo-format just tells the
+# qcow2 driver to read the actual format from the image header.
+#
 # Since: 2.10
 ##
 { 'union': 'ImageInfoSpecificQCow2Encryption',
@@ -2961,10 +2964,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: 4.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: 4.0
+##
+{ 'struct': 'BlockdevQcow2EncryptionSecret',
+  'data': { '*key-secret': 'str' } }
 
 ##
 # @BlockdevQcow2Encryption:
@@ -2972,10 +2995,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 c21452bc2f..5885f5175e 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -883,6 +883,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.20.1

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [Qemu-devel] [PATCH v3 5/8] qapi: Formalize qcow encryption probing
  2019-02-06 19:55 [Qemu-devel] [PATCH v3 0/8] block: Try to create well-typed json:{} filenames Max Reitz
                   ` (3 preceding siblings ...)
  2019-02-06 19:55 ` [Qemu-devel] [PATCH v3 4/8] qapi: Formalize qcow2 encryption probing Max Reitz
@ 2019-02-06 19:55 ` Max Reitz
  2019-02-06 19:55 ` [Qemu-devel] [PATCH v3 6/8] block: Try to create well typed json:{} filenames Max Reitz
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 22+ messages in thread
From: Max Reitz @ 2019-02-06 19:55 UTC (permalink / raw)
  To: qemu-block
  Cc: qemu-devel, Max Reitz, Eric Blake, Markus Armbruster,
	Michael Roth, Kevin Wolf

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>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@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 abf9af0c4d..8e193fe71f 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2939,8 +2939,9 @@
 # Since: 2.10
 ##
 { 'union': 'BlockdevQcowEncryption',
-  'base': { 'format': 'BlockdevQcowEncryptionFormat' },
+  'base': { '*format': 'BlockdevQcowEncryptionFormat' },
   'discriminator': 'format',
+  'default-variant': 'aes',
   'data': { 'aes': 'QCryptoBlockOptionsQCow' } }
 
 ##
-- 
2.20.1

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [Qemu-devel] [PATCH v3 6/8] block: Try to create well typed json:{} filenames
  2019-02-06 19:55 [Qemu-devel] [PATCH v3 0/8] block: Try to create well-typed json:{} filenames Max Reitz
                   ` (4 preceding siblings ...)
  2019-02-06 19:55 ` [Qemu-devel] [PATCH v3 5/8] qapi: Formalize qcow " Max Reitz
@ 2019-02-06 19:55 ` Max Reitz
  2019-02-06 20:43   ` Eric Blake
  2019-02-06 19:55 ` [Qemu-devel] [PATCH v3 7/8] iotests: Test internal option typing Max Reitz
  2019-02-06 19:55 ` [Qemu-devel] [PATCH v3 8/8] iotests: qcow2's encrypt.format is now optional Max Reitz
  7 siblings, 1 reply; 22+ messages in thread
From: Max Reitz @ 2019-02-06 19:55 UTC (permalink / raw)
  To: qemu-block
  Cc: qemu-devel, Max Reitz, Eric Blake, Markus Armbruster,
	Michael Roth, Kevin Wolf

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                    | 68 +++++++++++++++++++++++++++++++++++++-
 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, 78 insertions(+), 12 deletions(-)

diff --git a/block.c b/block.c
index d496debda4..c22f3fe5f1 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"
@@ -5581,6 +5582,56 @@ static bool bdrv_backing_overridden(BlockDriverState *bs)
     }
 }
 
+/**
+ * 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
@@ -5698,10 +5749,25 @@ void bdrv_refresh_filename(BlockDriverState *bs)
     if (bs->exact_filename[0]) {
         pstrcpy(bs->filename, sizeof(bs->filename), bs->exact_filename);
     } else {
-        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 46e6a60510..cb055ed39c 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 (actual path: TEST_DIR/t.IMGFMT.base)
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 45ac7c2a8f..b7d285a1e5 100644
--- a/tests/qemu-iotests/207.out
+++ b/tests/qemu-iotests/207.out
@@ -5,7 +5,7 @@
 {"execute": "job-dismiss", "arguments": {"id": "job0"}}
 {"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"}}
 {"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"}}
 {"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"}}
 {"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"}}
 {"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.20.1

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [Qemu-devel] [PATCH v3 7/8] iotests: Test internal option typing
  2019-02-06 19:55 [Qemu-devel] [PATCH v3 0/8] block: Try to create well-typed json:{} filenames Max Reitz
                   ` (5 preceding siblings ...)
  2019-02-06 19:55 ` [Qemu-devel] [PATCH v3 6/8] block: Try to create well typed json:{} filenames Max Reitz
@ 2019-02-06 19:55 ` Max Reitz
  2019-02-06 21:28   ` Eric Blake
  2019-02-06 19:55 ` [Qemu-devel] [PATCH v3 8/8] iotests: qcow2's encrypt.format is now optional Max Reitz
  7 siblings, 1 reply; 22+ messages in thread
From: Max Reitz @ 2019-02-06 19:55 UTC (permalink / raw)
  To: qemu-block
  Cc: qemu-devel, Max Reitz, Eric Blake, Markus Armbruster,
	Michael Roth, Kevin Wolf

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 3165d79e2a..0ac1e1fbf5 100755
--- a/tests/qemu-iotests/089
+++ b/tests/qemu-iotests/089
@@ -35,6 +35,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
@@ -144,6 +145,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.20.1

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [Qemu-devel] [PATCH v3 8/8] iotests: qcow2's encrypt.format is now optional
  2019-02-06 19:55 [Qemu-devel] [PATCH v3 0/8] block: Try to create well-typed json:{} filenames Max Reitz
                   ` (6 preceding siblings ...)
  2019-02-06 19:55 ` [Qemu-devel] [PATCH v3 7/8] iotests: Test internal option typing Max Reitz
@ 2019-02-06 19:55 ` Max Reitz
  2019-02-06 21:31   ` Eric Blake
  7 siblings, 1 reply; 22+ messages in thread
From: Max Reitz @ 2019-02-06 19:55 UTC (permalink / raw)
  To: qemu-block
  Cc: qemu-devel, Max Reitz, Eric Blake, Markus Armbruster,
	Michael Roth, Kevin Wolf

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>
Reviewed-by: Daniel P. Berrangé <berrange@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 f625887082..78c953ec50 100755
--- a/tests/qemu-iotests/087
+++ b/tests/qemu-iotests/087
@@ -149,7 +149,6 @@ run_qemu <<EOF
           "filename": "$TEST_IMG"
       },
       "encrypt": {
-          "format": "aes",
           "key-secret": "sec0"
       }
     }
@@ -161,34 +160,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 2d92ea847b..6c1d83519b 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, "reason": "host-qmp-quit"}}
 
+Testing:
+QMP_VERSION
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}}
+
+Testing:
+QMP_VERSION
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}}
+
+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, "reason": "host-qmp-quit"}}
+
 
 === Missing driver ===
 
-- 
2.20.1

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [Qemu-devel] [PATCH v3 1/8] qapi: Add default-variant for flat unions
  2019-02-06 19:55 ` [Qemu-devel] [PATCH v3 1/8] qapi: Add default-variant for flat unions Max Reitz
@ 2019-02-06 20:13   ` Eric Blake
  2019-02-07  6:56   ` Markus Armbruster
  1 sibling, 0 replies; 22+ messages in thread
From: Eric Blake @ 2019-02-06 20:13 UTC (permalink / raw)
  To: Max Reitz, qemu-block
  Cc: qemu-devel, Markus Armbruster, Michael Roth, Kevin Wolf

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

On 2/6/19 1:55 PM, Max Reitz wrote:
> 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         | 58 ++++++++++++++++++++++++++++------
>  scripts/qapi/doc.py            | 10 ++++--
>  scripts/qapi/introspect.py     | 10 ++++--
>  scripts/qapi/visit.py          | 13 ++++++++
>  tests/qapi-schema/test-qapi.py |  2 ++
>  6 files changed, 86 insertions(+), 15 deletions(-)

Missing a change to docs/devel/qapi-code-gen.txt? [re-reads diffstat in
cover letter...] Oh, you got that later in the series. Maybe those two
patches are worth squashing?

> +++ b/scripts/qapi/visit.py
> @@ -77,6 +77,19 @@ void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp)
>          ret += gen_endif(memb.ifcond)
>  
>      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))
> +

I like that you de-sugar it as early in the input parse as possible.

Markus may have comments, but it looks reasonable to me.

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

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


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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Qemu-devel] [PATCH v3 3/8] tests: Add QAPI optional discriminator tests
  2019-02-06 19:55 ` [Qemu-devel] [PATCH v3 3/8] tests: Add QAPI optional discriminator tests Max Reitz
@ 2019-02-06 20:20   ` Eric Blake
  0 siblings, 0 replies; 22+ messages in thread
From: Eric Blake @ 2019-02-06 20:20 UTC (permalink / raw)
  To: Max Reitz, qemu-block
  Cc: qemu-devel, Markus Armbruster, Michael Roth, Kevin Wolf

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

On 2/6/19 1:55 PM, Max Reitz wrote:
> 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>
> ---
Reviewed-by: Eric Blake <eblake@redhat.com>

This could also be squashed with patch 1 to avoid a window of 'make
check' breakage during a bisect; but as that makes the overall changes
bigger to review, leaving them separate is not too terrible.

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


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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Qemu-devel] [PATCH v3 4/8] qapi: Formalize qcow2 encryption probing
  2019-02-06 19:55 ` [Qemu-devel] [PATCH v3 4/8] qapi: Formalize qcow2 encryption probing Max Reitz
@ 2019-02-06 20:23   ` Eric Blake
  0 siblings, 0 replies; 22+ messages in thread
From: Eric Blake @ 2019-02-06 20:23 UTC (permalink / raw)
  To: Max Reitz, qemu-block
  Cc: qemu-devel, Markus Armbruster, Michael Roth, Kevin Wolf

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

On 2/6/19 1:55 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 | 31 ++++++++++++++++++++++++++++---
>  block/qcow2.c        |  3 +++
>  2 files changed, 31 insertions(+), 3 deletions(-)
> 

> @@ -2961,10 +2964,30 @@
>  # @BlockdevQcow2EncryptionFormat:
>  # @aes: AES-CBC with plain64 initialization venctors

We should fix that typo.

>  #
> +# @auto:            Determine the encryption format from the image
> +#                   header.  This only allows the use of the
> +#                   key-secret option.  (Since: 4.0)
> +#
>  # Since: 2.10

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

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


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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Qemu-devel] [PATCH v3 6/8] block: Try to create well typed json:{} filenames
  2019-02-06 19:55 ` [Qemu-devel] [PATCH v3 6/8] block: Try to create well typed json:{} filenames Max Reitz
@ 2019-02-06 20:43   ` Eric Blake
  2019-02-06 21:06     ` Eric Blake
  2019-02-08 18:11     ` Max Reitz
  0 siblings, 2 replies; 22+ messages in thread
From: Eric Blake @ 2019-02-06 20:43 UTC (permalink / raw)
  To: Max Reitz, qemu-block
  Cc: qemu-devel, Markus Armbruster, Michael Roth, Kevin Wolf

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

On 2/6/19 1:55 PM, Max Reitz wrote:

In the subject, s/well typed/well-typed/

> By applying a health mix of qdict_flatten(), qdict_crumple(),

s/health/healty/

> 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.

And someday, if we ever switch the block layer to use QAPI types all the
way, we can drop some of these hacks that have built up over time. But
not a show-stopper for this series.

> 
> 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                    | 68 +++++++++++++++++++++++++++++++++++++-
>  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, 78 insertions(+), 12 deletions(-)
> 

> +/**
> + * 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
> @@ -5698,10 +5749,25 @@ void bdrv_refresh_filename(BlockDriverState *bs)
>      if (bs->exact_filename[0]) {
>          pstrcpy(bs->filename, sizeof(bs->filename), bs->exact_filename);
>      } else {
> -        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));

I so need to revive my series that created a JSON output visitor (to
skip the qobject_to_json() intermediate step). But that's a topic for
another day.

> +++ 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"}'

"inject-error" sorts after "image"; meanwhile, the QAPI definition of
BlockdevOptionsBlkdebug sorts image before inject-error. It looks like
this output is randomized; can that bite us (as we've recently found
with other iotests where python 2 vs. 3 sorting mattered)?  And can we
do better?  (My QAPI JSON output visitor would guarantee the output in
QAPI definition order)

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

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


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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Qemu-devel] [PATCH v3 6/8] block: Try to create well typed json:{} filenames
  2019-02-06 20:43   ` Eric Blake
@ 2019-02-06 21:06     ` Eric Blake
  2019-02-08 18:11     ` Max Reitz
  1 sibling, 0 replies; 22+ messages in thread
From: Eric Blake @ 2019-02-06 21:06 UTC (permalink / raw)
  To: Max Reitz, qemu-block
  Cc: Michael Roth, Kevin Wolf, qemu-devel, Markus Armbruster

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

On 2/6/19 2:43 PM, Eric Blake wrote:
> On 2/6/19 1:55 PM, Max Reitz wrote:
> 
> In the subject, s/well typed/well-typed/
> 
>> By applying a health mix of qdict_flatten(), qdict_crumple(),
> 
> s/health/healty/

typo in my typo fix for healthy :)

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


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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Qemu-devel] [PATCH v3 7/8] iotests: Test internal option typing
  2019-02-06 19:55 ` [Qemu-devel] [PATCH v3 7/8] iotests: Test internal option typing Max Reitz
@ 2019-02-06 21:28   ` Eric Blake
  0 siblings, 0 replies; 22+ messages in thread
From: Eric Blake @ 2019-02-06 21:28 UTC (permalink / raw)
  To: Max Reitz, qemu-block
  Cc: qemu-devel, Markus Armbruster, Michael Roth, Kevin Wolf

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

On 2/6/19 1:55 PM, Max Reitz wrote:
> 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(+)
> 

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

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


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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Qemu-devel] [PATCH v3 8/8] iotests: qcow2's encrypt.format is now optional
  2019-02-06 19:55 ` [Qemu-devel] [PATCH v3 8/8] iotests: qcow2's encrypt.format is now optional Max Reitz
@ 2019-02-06 21:31   ` Eric Blake
  0 siblings, 0 replies; 22+ messages in thread
From: Eric Blake @ 2019-02-06 21:31 UTC (permalink / raw)
  To: Max Reitz, qemu-block
  Cc: qemu-devel, Markus Armbruster, Michael Roth, Kevin Wolf

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

On 2/6/19 1:55 PM, 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>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  tests/qemu-iotests/087     | 65 +++++++++++++++++++++++---------------
>  tests/qemu-iotests/087.out | 26 ++++++++++++++-
>  2 files changed, 64 insertions(+), 27 deletions(-)
> 
Reviewed-by: Eric Blake <eblake@redhat.com>

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


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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Qemu-devel] [PATCH v3 1/8] qapi: Add default-variant for flat unions
  2019-02-06 19:55 ` [Qemu-devel] [PATCH v3 1/8] qapi: Add default-variant for flat unions Max Reitz
  2019-02-06 20:13   ` Eric Blake
@ 2019-02-07  6:56   ` Markus Armbruster
  2019-02-07 14:01     ` Eric Blake
  2019-02-08 18:21     ` Max Reitz
  1 sibling, 2 replies; 22+ messages in thread
From: Markus Armbruster @ 2019-02-07  6:56 UTC (permalink / raw)
  To: Max Reitz; +Cc: qemu-block, Kevin Wolf, qemu-devel, Michael Roth

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.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  qapi/introspect.json           |  8 +++++
>  scripts/qapi/common.py         | 58 ++++++++++++++++++++++++++++------
>  scripts/qapi/doc.py            | 10 ++++--
>  scripts/qapi/introspect.py     | 10 ++++--
>  scripts/qapi/visit.py          | 13 ++++++++
>  tests/qapi-schema/test-qapi.py |  2 ++
>  6 files changed, 86 insertions(+), 15 deletions(-)
>
> diff --git a/qapi/introspect.json b/qapi/introspect.json
> index 3d22166b2b..e4740aa7a0 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: 4.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' ] } }
>  
>  ##

I'm afraid this could become awkward later on.  Let me explain.

In many programming languages, absent optional arguments / members
default to a default value specified in the declaration.  Simple.

In others, 'absent' is effectively an additional value.  The code
consuming the argument / member can interpret 'absent' however it wants.
It may eliminate the additional value by mapping it to a default value,
but it can also interpret 'absent' unlike any value.  If there's more
than one consumer, their interpretations need not be consistent.  The
declaration provides no clue on semantics of 'absent'.

QAPI is in the latter camp.  I trust you can already sense how much I
like that.

Now you want to permit optional variant discriminators.  As per general
rule, their interpretation is left to the code consuming it.  One
instance of such code is the generated union visitor, which needs to
decide which variant to visit.  Your default-variant tells it which
variant to visit.  Other code interpreting the discriminator better be
consistent with that, but that's the other code's problem.  Hmm.

If I could go back in time, I'd flip QAPI to "'absent' defaults to a
default value".  Lacking a time machine, we're stuck with cases of
"'absent' means something you can't express with a value" and "'absent'
means different things in different contexts" that have been enshrined
in external interfaces.  Is there anything we could do to improve
matters for saner cases?

I think there is: we could provide for an *optional* default value.  If
the schema specifies it, that's what 'absent' means.  If it doesn't, all
bets are off, just like they are now.

Say we do that (for what it's worth, introspect.json is already prepared
for it).  How would it play with your default-variant?

If an optional discriminator specifies a default value, then that's the
default variant.  But wait, if there's also a default-variant, *that's*
the default variant!  Awkward clash.  To resolve it, we could either
forbid combining the two, or rule default-variant overrides the default.
Feels needlessly complicated.

Could we get away with "if you want a default variant, the discriminator
must specify a default"?

[...]

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Qemu-devel] [PATCH v3 1/8] qapi: Add default-variant for flat unions
  2019-02-07  6:56   ` Markus Armbruster
@ 2019-02-07 14:01     ` Eric Blake
  2019-02-07 14:46       ` Eric Blake
  2019-02-08 18:21     ` Max Reitz
  1 sibling, 1 reply; 22+ messages in thread
From: Eric Blake @ 2019-02-07 14:01 UTC (permalink / raw)
  To: Markus Armbruster, Max Reitz
  Cc: Kevin Wolf, qemu-devel, qemu-block, Michael Roth

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

On 2/7/19 12:56 AM, 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.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---

> I'm afraid this could become awkward later on.  Let me explain.
> 
> In many programming languages, absent optional arguments / members
> default to a default value specified in the declaration.  Simple.
> 
> In others, 'absent' is effectively an additional value.  The code
> consuming the argument / member can interpret 'absent' however it wants.
> It may eliminate the additional value by mapping it to a default value,
> but it can also interpret 'absent' unlike any value.  If there's more
> than one consumer, their interpretations need not be consistent.  The
> declaration provides no clue on semantics of 'absent'.
> 
> QAPI is in the latter camp.  I trust you can already sense how much I
> like that.
> 
> Now you want to permit optional variant discriminators.  As per general
> rule, their interpretation is left to the code consuming it.  One
> instance of such code is the generated union visitor, which needs to
> decide which variant to visit.  Your default-variant tells it which
> variant to visit.  Other code interpreting the discriminator better be
> consistent with that, but that's the other code's problem.  Hmm.
> 
> If I could go back in time, I'd flip QAPI to "'absent' defaults to a
> default value".  Lacking a time machine, we're stuck with cases of
> "'absent' means something you can't express with a value" and "'absent'
> means different things in different contexts" that have been enshrined
> in external interfaces.  Is there anything we could do to improve
> matters for saner cases?
> 
> I think there is: we could provide for an *optional* default value.  If
> the schema specifies it, that's what 'absent' means.  If it doesn't, all
> bets are off, just like they are now.

And we already have the planned syntax, thanks to our recent work on
adding conditionals - where we now have:

{ '*field': 'mytype' }

we can also do long-hand:

{ { 'name': '*field', 'type': 'mytype' } }

which also lends itself well to declaring a default:

{ { 'name': '*field', 'type': 'mytype', 'default': 'xyz' } }

> 
> Say we do that (for what it's worth, introspect.json is already prepared
> for it).  How would it play with your default-variant?
> 
> If an optional discriminator specifies a default value, then that's the
> default variant.  But wait, if there's also a default-variant, *that's*
> the default variant!  Awkward clash.  To resolve it, we could either
> forbid combining the two, or rule default-variant overrides the default.
> Feels needlessly complicated.
> 
> Could we get away with "if you want a default variant, the discriminator
> must specify a default"?

I like the idea. It means finally biting the bullet and implementing
default values, but we've known we've wanted them for a while (as
evidenced by the existing introspection output, and by the syntax that
we have ready to put into use for it).

It means that representing the default variant in a union now depends on
the base class declaring a default for the optional parameter (that is,
an optional parameter can only be used as discriminator if it has a
declared default), and gives us variable defaults in more uses than just
unions.

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


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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Qemu-devel] [PATCH v3 1/8] qapi: Add default-variant for flat unions
  2019-02-07 14:01     ` Eric Blake
@ 2019-02-07 14:46       ` Eric Blake
  0 siblings, 0 replies; 22+ messages in thread
From: Eric Blake @ 2019-02-07 14:46 UTC (permalink / raw)
  To: Markus Armbruster, Max Reitz
  Cc: Kevin Wolf, qemu-devel, qemu-block, Michael Roth

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

On 2/7/19 8:01 AM, Eric Blake wrote:

>> I think there is: we could provide for an *optional* default value.  If
>> the schema specifies it, that's what 'absent' means.  If it doesn't, all
>> bets are off, just like they are now.
> 
> And we already have the planned syntax, thanks to our recent work on
> adding conditionals - where we now have:
> 
> { '*field': 'mytype' }
> 
> we can also do long-hand:
> 
> { { 'name': '*field', 'type': 'mytype' } }

I'd better use the actual syntax, instead of inventing non-JSON off the
top of my head:

{ '*field': { 'type': 'mytype' } }

> 
> which also lends itself well to declaring a default:
> 
> { { 'name': '*field', 'type': 'mytype', 'default': 'xyz' } }

{ '*field': { 'type': 'mytype', 'default': 'xyz' } }

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


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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Qemu-devel] [PATCH v3 6/8] block: Try to create well typed json:{} filenames
  2019-02-06 20:43   ` Eric Blake
  2019-02-06 21:06     ` Eric Blake
@ 2019-02-08 18:11     ` Max Reitz
  1 sibling, 0 replies; 22+ messages in thread
From: Max Reitz @ 2019-02-08 18:11 UTC (permalink / raw)
  To: Eric Blake, qemu-block
  Cc: qemu-devel, Markus Armbruster, Michael Roth, Kevin Wolf

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

On 06.02.19 21:43, Eric Blake wrote:
> On 2/6/19 1:55 PM, Max Reitz wrote:
> 
> In the subject, s/well typed/well-typed/
> 
>> By applying a health mix of qdict_flatten(), qdict_crumple(),
> 
> s/health/healty/
> 
>> qdict_stringify_for_keyval(), the keyval input visitor, and the QObject
>> output visitor (not necessarily in that order),

More importantly, this is just wrong (or rather, outdated).  I fixed it
in the cover letter but forgot to fix it here.  This version just uses
qdict_flatten(), the flat-confused input visitor and the output visitor.
 There is no longer a qdict_stringify_for_keyval() because Markus's
flat-confused visitor made it unnecessary.

>>                                                 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.
> 
> And someday, if we ever switch the block layer to use QAPI types all the
> way, we can drop some of these hacks that have built up over time. But
> not a show-stopper for this series.
> 
>>
>> 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                    | 68 +++++++++++++++++++++++++++++++++++++-
>>  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, 78 insertions(+), 12 deletions(-)
>>
> 
>> +/**
>> + * 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
>> @@ -5698,10 +5749,25 @@ void bdrv_refresh_filename(BlockDriverState *bs)
>>      if (bs->exact_filename[0]) {
>>          pstrcpy(bs->filename, sizeof(bs->filename), bs->exact_filename);
>>      } else {
>> -        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));
> 
> I so need to revive my series that created a JSON output visitor (to
> skip the qobject_to_json() intermediate step). But that's a topic for
> another day.
> 
>> +++ 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"}'
> 
> "inject-error" sorts after "image"; meanwhile, the QAPI definition of
> BlockdevOptionsBlkdebug sorts image before inject-error. It looks like
> this output is randomized; can that bite us (as we've recently found
> with other iotests where python 2 vs. 3 sorting mattered)?  And can we
> do better?  (My QAPI JSON output visitor would guarantee the output in
> QAPI definition order)

It can't bite us in the Python 2 vs. 3 sense because the order is
determined by qemu code alone (so it is stable for one version of qemu),
but of course a real order would be nicer.

I don't think it makes sense to parse the filename in the iotest (for
one thing, we'd need to convert this test to Python for that) so we'd be
able to guarantee some order in the output, so I think it's best to just
live with it for now.

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

Once again, thanks!

Max


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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Qemu-devel] [PATCH v3 1/8] qapi: Add default-variant for flat unions
  2019-02-07  6:56   ` Markus Armbruster
  2019-02-07 14:01     ` Eric Blake
@ 2019-02-08 18:21     ` Max Reitz
  2019-06-05 19:00       ` Eric Blake
  1 sibling, 1 reply; 22+ messages in thread
From: Max Reitz @ 2019-02-08 18:21 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: qemu-block, Kevin Wolf, qemu-devel, Michael Roth

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

On 07.02.19 07:56, 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.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  qapi/introspect.json           |  8 +++++
>>  scripts/qapi/common.py         | 58 ++++++++++++++++++++++++++++------
>>  scripts/qapi/doc.py            | 10 ++++--
>>  scripts/qapi/introspect.py     | 10 ++++--
>>  scripts/qapi/visit.py          | 13 ++++++++
>>  tests/qapi-schema/test-qapi.py |  2 ++
>>  6 files changed, 86 insertions(+), 15 deletions(-)
>>
>> diff --git a/qapi/introspect.json b/qapi/introspect.json
>> index 3d22166b2b..e4740aa7a0 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: 4.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' ] } }
>>  
>>  ##
> 
> I'm afraid this could become awkward later on.  Let me explain.
> 
> In many programming languages, absent optional arguments / members
> default to a default value specified in the declaration.  Simple.
> 
> In others, 'absent' is effectively an additional value.  The code
> consuming the argument / member can interpret 'absent' however it wants.
> It may eliminate the additional value by mapping it to a default value,
> but it can also interpret 'absent' unlike any value.  If there's more
> than one consumer, their interpretations need not be consistent.  The
> declaration provides no clue on semantics of 'absent'.
> 
> QAPI is in the latter camp.  I trust you can already sense how much I
> like that.
> 
> Now you want to permit optional variant discriminators.  As per general
> rule, their interpretation is left to the code consuming it.  One
> instance of such code is the generated union visitor, which needs to
> decide which variant to visit.  Your default-variant tells it which
> variant to visit.  Other code interpreting the discriminator better be
> consistent with that, but that's the other code's problem.  Hmm.
> 
> If I could go back in time, I'd flip QAPI to "'absent' defaults to a
> default value".  Lacking a time machine, we're stuck with cases of
> "'absent' means something you can't express with a value" and "'absent'
> means different things in different contexts" that have been enshrined
> in external interfaces.  Is there anything we could do to improve
> matters for saner cases?
> 
> I think there is: we could provide for an *optional* default value.  If
> the schema specifies it, that's what 'absent' means.  If it doesn't, all
> bets are off, just like they are now.
> 
> Say we do that (for what it's worth, introspect.json is already prepared
> for it).

If somebody(tm) does that, great. :-)

> How would it play with your default-variant?
> 
> If an optional discriminator specifies a default value, then that's the
> default variant.  But wait, if there's also a default-variant, *that's*
> the default variant!  Awkward clash.  To resolve it, we could either
> forbid combining the two, or rule default-variant overrides the default.
> Feels needlessly complicated.

I agree on the needless, not so sure about the complicated.  (Other than
it being double the work, but, well, the default-variant work is already
right here.)

> Could we get away with "if you want a default variant, the discriminator
> must specify a default"?

I think so, yes.  I agree that it'd be the better solution.  But to be
honest I'd really rather not delve any deeper into the QAPI dungeon than
I've done in this series so far...

Max


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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Qemu-devel] [PATCH v3 1/8] qapi: Add default-variant for flat unions
  2019-02-08 18:21     ` Max Reitz
@ 2019-06-05 19:00       ` Eric Blake
  0 siblings, 0 replies; 22+ messages in thread
From: Eric Blake @ 2019-06-05 19:00 UTC (permalink / raw)
  To: Max Reitz, Markus Armbruster
  Cc: Kevin Wolf, qemu-devel, qemu-block, Michael Roth

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

On 2/8/19 12:21 PM, Max Reitz wrote:
> On 07.02.19 07:56, 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.
>>>
>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>> ---

>>> +++ 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: 4.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' ] } }
>>>  
>>>  ##
>>
>> I'm afraid this could become awkward later on.  Let me explain.
>>
>> In many programming languages, absent optional arguments / members
>> default to a default value specified in the declaration.  Simple.
>>
>> In others, 'absent' is effectively an additional value.  The code
>> consuming the argument / member can interpret 'absent' however it wants.
>> It may eliminate the additional value by mapping it to a default value,
>> but it can also interpret 'absent' unlike any value.  If there's more
>> than one consumer, their interpretations need not be consistent.  The
>> declaration provides no clue on semantics of 'absent'.
>>
>> QAPI is in the latter camp.  I trust you can already sense how much I
>> like that.
>>
>> Now you want to permit optional variant discriminators.  As per general
>> rule, their interpretation is left to the code consuming it.  One
>> instance of such code is the generated union visitor, which needs to
>> decide which variant to visit.  Your default-variant tells it which
>> variant to visit.  Other code interpreting the discriminator better be
>> consistent with that, but that's the other code's problem.  Hmm.
>>
>> If I could go back in time, I'd flip QAPI to "'absent' defaults to a
>> default value".  Lacking a time machine, we're stuck with cases of
>> "'absent' means something you can't express with a value" and "'absent'
>> means different things in different contexts" that have been enshrined
>> in external interfaces.  Is there anything we could do to improve
>> matters for saner cases?
>>
>> I think there is: we could provide for an *optional* default value.  If
>> the schema specifies it, that's what 'absent' means.  If it doesn't, all
>> bets are off, just like they are now.
>>
>> Say we do that (for what it's worth, introspect.json is already prepared
>> for it).
> 
> If somebody(tm) does that, great. :-)
> 
>> How would it play with your default-variant?
>>
>> If an optional discriminator specifies a default value, then that's the
>> default variant.  But wait, if there's also a default-variant, *that's*
>> the default variant!  Awkward clash.  To resolve it, we could either
>> forbid combining the two, or rule default-variant overrides the default.
>> Feels needlessly complicated.
> 
> I agree on the needless, not so sure about the complicated.  (Other than
> it being double the work, but, well, the default-variant work is already
> right here.)
> 
>> Could we get away with "if you want a default variant, the discriminator
>> must specify a default"?
> 
> I think so, yes.  I agree that it'd be the better solution.  But to be
> honest I'd really rather not delve any deeper into the QAPI dungeon than
> I've done in this series so far...

I've now hit a case where I'd like a default-variant (namely, improving
nbd-server-add to avoid SocketAddressLegacy); maybe I'll find time to
revive at least this part of the series.

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


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

^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2019-06-05 19:02 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-06 19:55 [Qemu-devel] [PATCH v3 0/8] block: Try to create well-typed json:{} filenames Max Reitz
2019-02-06 19:55 ` [Qemu-devel] [PATCH v3 1/8] qapi: Add default-variant for flat unions Max Reitz
2019-02-06 20:13   ` Eric Blake
2019-02-07  6:56   ` Markus Armbruster
2019-02-07 14:01     ` Eric Blake
2019-02-07 14:46       ` Eric Blake
2019-02-08 18:21     ` Max Reitz
2019-06-05 19:00       ` Eric Blake
2019-02-06 19:55 ` [Qemu-devel] [PATCH v3 2/8] docs/qapi: Document optional discriminators Max Reitz
2019-02-06 19:55 ` [Qemu-devel] [PATCH v3 3/8] tests: Add QAPI optional discriminator tests Max Reitz
2019-02-06 20:20   ` Eric Blake
2019-02-06 19:55 ` [Qemu-devel] [PATCH v3 4/8] qapi: Formalize qcow2 encryption probing Max Reitz
2019-02-06 20:23   ` Eric Blake
2019-02-06 19:55 ` [Qemu-devel] [PATCH v3 5/8] qapi: Formalize qcow " Max Reitz
2019-02-06 19:55 ` [Qemu-devel] [PATCH v3 6/8] block: Try to create well typed json:{} filenames Max Reitz
2019-02-06 20:43   ` Eric Blake
2019-02-06 21:06     ` Eric Blake
2019-02-08 18:11     ` Max Reitz
2019-02-06 19:55 ` [Qemu-devel] [PATCH v3 7/8] iotests: Test internal option typing Max Reitz
2019-02-06 21:28   ` Eric Blake
2019-02-06 19:55 ` [Qemu-devel] [PATCH v3 8/8] iotests: qcow2's encrypt.format is now optional Max Reitz
2019-02-06 21:31   ` Eric Blake

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.