Re: [Qemu-block] [PATCH v4 00/14] block: Try to create well-typed json:{} filenames

[Max Reitz]

Another gentle ping.

Max


On 24.06.19 19:39, Max Reitz wrote:
> Hi,
> 
> There are two explanations of this cover letter, a relative one (to v3)
> and an absolute one.
> 
> 
> *** Important note ***
> 
> The final patch in this series is an example that converts most of
> block-core.json to use default values where possible.  We may decide to
> take it or not.  It isn’t important for the main purpose of this series,
> so I’d be very much fine with chopping it off.
> 
> (It does have a nice diff stat, though.)
> 
> *** Important note end ***
> 
> 
> Relative explanation:
> 
> The actual functional goal of this series is to allow all blockdev
> options that can be represented with -drive to have an equivalent with
> -blockdev (safe for rbd’s =keyvalue-pairs).
> 
> To this end, qcow(2)’s encryption needs an “auto” format which can
> automatically deduce the format from the image header.  To make things
> nicer, I decided (already in v1) to make this format optional so users
> could just specify encrypt.secret and let the format driver figure out
> the rest.
> 
> Until v3, this was implemented by letting the discriminator of flat
> unions be optional, as long as a default-value is provided.  Markus
> (rightfully) complained that this is very specific and would be covered
> by just having default values for QAPI struct members in general.
> So now this version implements this.  This is a bit more complicated
> than just implementing a default-variant, mainly because the latter only
> needs to accept enum values, whereas a generally usable “default” should
> accept values of all QAPI types (to the extent what is reasonable).
> 
> So what was (until v3)
> 
>   { 'union': 'Foo',
>     'base': { '*discr': 'SomeEnum' },
>     'discriminator': 'discr',
>     'default-variant': 'value1',
>     'data': { 'value1': 'Bar', 'value2': 'Baz' } }
> 
> becomes
> 
>   { 'union': 'Foo',
>     'base': { '*discr': { 'type': 'SomeEnum', 'default': 'value1' } },
>     'discriminator': 'discr',
>     'data': { 'value1': 'Bar', 'value2': 'Baz' } }
> 
> 
> 
> Absolute explanation:
> 
> 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 11, 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.  This is accomplished by
> generally allowing default values to be provided for QAPI struct
> members.
> 
> 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 default values for
>   optional struct members.  This in turn allows flat union
>   discriminators be optional, too, but only if a default value is
>   provided.
>   - 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.)
> 
> 
> v4:
> - Drop the default-variant stuff and replace it by a more general
>   concept of allowing default values for all QAPI struct members
> 
> 
> git backport-diff against v3:
> 
> 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/14:[down] 'qapi: Parse numeric values'
> 002/14:[down] 'qapi: Move to_c_string() to common.py'
> 003/14:[down] 'qapi: Introduce default values for struct members'
> 004/14:[down] 'qapi: Allow optional discriminators'
> 005/14:[down] 'qapi: Document default values for struct members'
> 006/14:[down] 'test-qapi: Print struct members' default values'
> 007/14:[down] 'tests: Test QAPI default values for struct members'
> 008/14:[0044] [FC] 'tests: Add QAPI optional discriminator tests'
> 009/14:[0009] [FC] 'qapi: Formalize qcow2 encryption probing'
> 010/14:[0005] [FC] 'qapi: Formalize qcow encryption probing'
> 011/14:[0014] [FC] 'block: Try to create well typed json:{} filenames'
> 012/14:[----] [--] 'iotests: Test internal option typing'
> 013/14:[----] [--] 'iotests: qcow2's encrypt.format is now optional'
> 014/14:[down] 'block: Make use of QAPI defaults'
> 
> 
> Max Reitz (14):
>   qapi: Parse numeric values
>   qapi: Move to_c_string() to common.py
>   qapi: Introduce default values for struct members
>   qapi: Allow optional discriminators
>   qapi: Document default values for struct members
>   test-qapi: Print struct members' default values
>   tests: Test QAPI default values for struct members
>   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
>   block: Make use of QAPI defaults
> 
>  docs/devel/qapi-code-gen.txt                  |  81 +++++-
>  tests/Makefile.include                        |  17 +-
>  qapi/block-core.json                          | 180 +++++++++-----
>  qapi/introspect.json                          |   9 +-
>  tests/qapi-schema/bad-type-int.json           |   1 -
>  tests/qapi-schema/enum-int-member.json        |   1 -
>  ...l-discriminator-invalid-specification.json |  11 +
>  ...on-optional-discriminator-no-default.json} |   5 +-
>  tests/qapi-schema/qapi-schema-test.json       |  38 +++
>  .../struct-member-alternate-default.json      |  10 +
>  ...struct-member-bool-wrong-default-type.json |   3 +
>  .../struct-member-enum-invalid-default.json   |   4 +
>  ...struct-member-enum-wrong-default-type.json |   4 +
>  .../struct-member-float-invalid-default.json  |   4 +
>  ...truct-member-float-wrong-default-type.json |   3 +
>  .../struct-member-int-wrong-default-type.json |   3 +
>  .../struct-member-int8-erange-default.json    |   3 +
>  .../struct-member-list-nonempty-default.json  |   4 +
>  .../struct-member-non-optional-default.json   |   3 +
>  .../struct-member-null-default.json           |   6 +
>  .../struct-member-str-wrong-default-type.json |   3 +
>  .../struct-member-uint8-erange-default.json   |   3 +
>  .../struct-member-uint8-negative-default.json |   3 +
>  block.c                                       |  68 ++++-
>  block/file-posix.c                            |   9 -
>  block/file-win32.c                            |   8 +-
>  block/parallels.c                             |   6 +-
>  block/qcow2.c                                 |  39 +--
>  block/qed.c                                   |   3 -
>  block/sheepdog.c                              |   3 -
>  block/vdi.c                                   |   3 -
>  block/vhdx.c                                  |  28 +--
>  block/vpc.c                                   |   3 -
>  blockdev.c                                    | 182 +++-----------
>  monitor/hmp-cmds.c                            |  27 +-
>  monitor/qmp-cmds.c                            |   3 +-
>  scripts/qapi/commands.py                      |   2 +-
>  scripts/qapi/common.py                        | 232 ++++++++++++++++--
>  scripts/qapi/doc.py                           |  20 +-
>  scripts/qapi/introspect.py                    |   8 +-
>  scripts/qapi/types.py                         |   2 +-
>  scripts/qapi/visit.py                         |  38 ++-
>  tests/qapi-schema/bad-type-int.err            |   2 +-
>  tests/qapi-schema/enum-int-member.err         |   2 +-
>  ...al-discriminator-invalid-specification.err |   1 +
>  ...-discriminator-invalid-specification.exit} |   0
>  ...l-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 -
>  tests/qapi-schema/leading-comma-list.err      |   2 +-
>  tests/qapi-schema/qapi-schema-test.out        |  33 +++
>  .../struct-member-alternate-default.err       |   1 +
>  .../struct-member-alternate-default.exit      |   1 +
>  .../struct-member-alternate-default.out       |   0
>  .../struct-member-bool-wrong-default-type.err |   1 +
>  ...struct-member-bool-wrong-default-type.exit |   1 +
>  .../struct-member-bool-wrong-default-type.out |   0
>  .../struct-member-enum-invalid-default.err    |   1 +
>  .../struct-member-enum-invalid-default.exit   |   1 +
>  .../struct-member-enum-invalid-default.out    |   0
>  .../struct-member-enum-wrong-default-type.err |   1 +
>  ...struct-member-enum-wrong-default-type.exit |   1 +
>  .../struct-member-enum-wrong-default-type.out |   0
>  .../struct-member-float-invalid-default.err   |   1 +
>  .../struct-member-float-invalid-default.exit  |   1 +
>  .../struct-member-float-invalid-default.out   |   0
>  ...struct-member-float-wrong-default-type.err |   1 +
>  ...truct-member-float-wrong-default-type.exit |   1 +
>  ...struct-member-float-wrong-default-type.out |   0
>  .../struct-member-int-wrong-default-type.err  |   1 +
>  .../struct-member-int-wrong-default-type.exit |   1 +
>  .../struct-member-int-wrong-default-type.out  |   0
>  .../struct-member-int8-erange-default.err     |   1 +
>  .../struct-member-int8-erange-default.exit    |   1 +
>  .../struct-member-int8-erange-default.out     |   0
>  .../struct-member-list-nonempty-default.err   |   1 +
>  .../struct-member-list-nonempty-default.exit  |   1 +
>  .../struct-member-list-nonempty-default.out   |   0
>  .../struct-member-non-optional-default.err    |   1 +
>  .../struct-member-non-optional-default.exit   |   1 +
>  .../struct-member-non-optional-default.out    |   0
>  .../struct-member-null-default.err            |   1 +
>  .../struct-member-null-default.exit           |   1 +
>  .../struct-member-null-default.out            |   0
>  .../struct-member-str-wrong-default-type.err  |   1 +
>  .../struct-member-str-wrong-default-type.exit |   1 +
>  .../struct-member-str-wrong-default-type.out  |   0
>  .../struct-member-uint8-erange-default.err    |   1 +
>  .../struct-member-uint8-erange-default.exit   |   1 +
>  .../struct-member-uint8-erange-default.out    |   0
>  .../struct-member-uint8-negative-default.err  |   1 +
>  .../struct-member-uint8-negative-default.exit |   1 +
>  .../struct-member-uint8-negative-default.out  |   0
>  tests/qapi-schema/test-qapi.py                |   8 +-
>  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 +-
>  104 files changed, 915 insertions(+), 384 deletions(-)
>  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} (68%)
>  create mode 100644 tests/qapi-schema/struct-member-alternate-default.json
>  create mode 100644 
> tests/qapi-schema/struct-member-bool-wrong-default-type.json
>  create mode 100644 tests/qapi-schema/struct-member-enum-invalid-default.json
>  create mode 100644 
> tests/qapi-schema/struct-member-enum-wrong-default-type.json
>  create mode 100644 tests/qapi-schema/struct-member-float-invalid-default.json
>  create mode 100644 
> tests/qapi-schema/struct-member-float-wrong-default-type.json
>  create mode 100644 
> tests/qapi-schema/struct-member-int-wrong-default-type.json
>  create mode 100644 tests/qapi-schema/struct-member-int8-erange-default.json
>  create mode 100644 tests/qapi-schema/struct-member-list-nonempty-default.json
>  create mode 100644 tests/qapi-schema/struct-member-non-optional-default.json
>  create mode 100644 tests/qapi-schema/struct-member-null-default.json
>  create mode 100644 
> tests/qapi-schema/struct-member-str-wrong-default-type.json
>  create mode 100644 tests/qapi-schema/struct-member-uint8-erange-default.json
>  create mode 100644 
> tests/qapi-schema/struct-member-uint8-negative-default.json
>  create mode 100644 
> tests/qapi-schema/flat-union-optional-discriminator-invalid-specification.err
>  rename tests/qapi-schema/{flat-union-optional-discriminator.exit => 
> flat-union-optional-discriminator-invalid-specification.exit} (100%)
>  rename tests/qapi-schema/{flat-union-optional-discriminator.out => 
> flat-union-optional-discriminator-invalid-specification.out} (100%)
>  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/struct-member-alternate-default.err
>  create mode 100644 tests/qapi-schema/struct-member-alternate-default.exit
>  create mode 100644 tests/qapi-schema/struct-member-alternate-default.out
>  create mode 100644 
> tests/qapi-schema/struct-member-bool-wrong-default-type.err
>  create mode 100644 
> tests/qapi-schema/struct-member-bool-wrong-default-type.exit
>  create mode 100644 
> tests/qapi-schema/struct-member-bool-wrong-default-type.out
>  create mode 100644 tests/qapi-schema/struct-member-enum-invalid-default.err
>  create mode 100644 tests/qapi-schema/struct-member-enum-invalid-default.exit
>  create mode 100644 tests/qapi-schema/struct-member-enum-invalid-default.out
>  create mode 100644 
> tests/qapi-schema/struct-member-enum-wrong-default-type.err
>  create mode 100644 
> tests/qapi-schema/struct-member-enum-wrong-default-type.exit
>  create mode 100644 
> tests/qapi-schema/struct-member-enum-wrong-default-type.out
>  create mode 100644 tests/qapi-schema/struct-member-float-invalid-default.err
>  create mode 100644 tests/qapi-schema/struct-member-float-invalid-default.exit
>  create mode 100644 tests/qapi-schema/struct-member-float-invalid-default.out
>  create mode 100644 
> tests/qapi-schema/struct-member-float-wrong-default-type.err
>  create mode 100644 
> tests/qapi-schema/struct-member-float-wrong-default-type.exit
>  create mode 100644 
> tests/qapi-schema/struct-member-float-wrong-default-type.out
>  create mode 100644 tests/qapi-schema/struct-member-int-wrong-default-type.err
>  create mode 100644 
> tests/qapi-schema/struct-member-int-wrong-default-type.exit
>  create mode 100644 tests/qapi-schema/struct-member-int-wrong-default-type.out
>  create mode 100644 tests/qapi-schema/struct-member-int8-erange-default.err
>  create mode 100644 tests/qapi-schema/struct-member-int8-erange-default.exit
>  create mode 100644 tests/qapi-schema/struct-member-int8-erange-default.out
>  create mode 100644 tests/qapi-schema/struct-member-list-nonempty-default.err
>  create mode 100644 tests/qapi-schema/struct-member-list-nonempty-default.exit
>  create mode 100644 tests/qapi-schema/struct-member-list-nonempty-default.out
>  create mode 100644 tests/qapi-schema/struct-member-non-optional-default.err
>  create mode 100644 tests/qapi-schema/struct-member-non-optional-default.exit
>  create mode 100644 tests/qapi-schema/struct-member-non-optional-default.out
>  create mode 100644 tests/qapi-schema/struct-member-null-default.err
>  create mode 100644 tests/qapi-schema/struct-member-null-default.exit
>  create mode 100644 tests/qapi-schema/struct-member-null-default.out
>  create mode 100644 tests/qapi-schema/struct-member-str-wrong-default-type.err
>  create mode 100644 
> tests/qapi-schema/struct-member-str-wrong-default-type.exit
>  create mode 100644 tests/qapi-schema/struct-member-str-wrong-default-type.out
>  create mode 100644 tests/qapi-schema/struct-member-uint8-erange-default.err
>  create mode 100644 tests/qapi-schema/struct-member-uint8-erange-default.exit
>  create mode 100644 tests/qapi-schema/struct-member-uint8-erange-default.out
>  create mode 100644 tests/qapi-schema/struct-member-uint8-negative-default.err
>  create mode 100644 
> tests/qapi-schema/struct-member-uint8-negative-default.exit
>  create mode 100644 tests/qapi-schema/struct-member-uint8-negative-default.out
>

signature.asc
Description: OpenPGP digital signature