[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[Qemu-devel] [PATCH 00/13] block: Try to create well typed json:{} filen

From: Max Reitz
Subject: [Qemu-devel] [PATCH 00/13] block: Try to create well typed json:{} filenames
Date: Wed, 9 May 2018 18:55:17 +0200

The “Try” in the subject of this series means more or less the same as
the “Try” in the subject line of
http://lists.nongnu.org/archive/html/qemu-block/2018-05/msg00061.html .

The difference is that it’s not so important here, because the worst
outcome is that in very narrow edge cases[1] (e.g. you trying to use
password-secret for rbd) we fail to generate a properly typed json:{}
filename -- but that is no regression over what we currently have.  In
the vast majority of cases[1], this series will result in proper

Or, well, I should add that I’m not quite sure about all the edge
cases[1], solely because the current bdrv_refresh_filename() code is a
mess, especially when it comes to gathering the blockdev options.  But
I do know that after my series to address that (“block: Fix some
filename generation issues”), it only comes down to a very small number
of edge cases (the only hard edge case is rbd’s =keyvalue-pairs, but
that is kaput anyway).[1]

But since this series does not have any hard requirement on that other
one, I decided to send it independently.

[1] You might have noticed a number of [1]s above.  This is because the
above text is as it was in a version of this series with only five
patches in it.  Then I noticed a not-so-edge case where converting to
QAPI broke down: qcow(2) encryption.  Since qcow and qcow2 now support
both the old AES and luks encryption, and every encryption type may
offer its own runtime options, you now have a flat union there (under
“encrypt”) which is discriminated based on encrypt.format.
Now the qcow2 driver can of course detect the format from the image
header, so you don’t really have to specify it.  In fact, when your
options don’t go through QAPI, you actually don’t have to.  So this is
exactly the not-so-edge case I was talking about.

QAPI cannot parse encrypt before it knows encrypt.format.  So it’s much
too late when the qcow2 driver detects encrypt.format in its
.bdrv_open() plementation.

I’m more than happy to take alternative suggestions, but this is an
issue we really cannot ignore (unlike =keyvalue-pairs), so the two
solutions I came up with are:

(1) Say auto-detection was a bad idea and make encrypt.format mandatory,
    even without QAPI.  This is always a possibility, but it does seem
    kind of cumbersome to users, so I wanted to explore the other option
    first, not knowing how deep into the rabbit hole that would lead.

(2) Allow flat union discriminators to be optional.  Since both AES and
    luks encryption in practice only allow a key-secret option, we can
    add a new runtime encryption pseudo-type, which is “from-image” that
    only allows such a key-secret option.  Now, when the user does not
    specify encrypt.format, the format is assumed to be “from-image”
    which means that you can specify a key-secret and qcow/qcow2 will
    figure out what encryption to use.
    This allows to retain compatibility with the current interface, but
    it also means this series exploded, and maybe the idea is
    universally hated.
    One issue I can see with this approach is how it would fare with new
    encryption types added; specifically, what if we add some encryption
    type that does not have a key-secret but something else?  Do we add
    that something else to from-image and make everything optional?  Or
    do we require users to explicitly specify the encryption type for
    such a new type when it is not compatible with from-image?
    In any case, one thing we have to watch out for from now on is that
    drivers must not allow options to be optional that are mandatory in
    the QAPI schema.
    More generally, we have to make sure that the driver’s interface on
    -drive is compatible to QAPI.  But I guess everybody knows that, so
    it’s good to be a bit more specific and say that this doesn’t just
    mean that options must match.  Mandatoriness must match also.

So, OK, obviously I chose solution (2) for now.  What this means is that
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.

- qdict_stringify_for_keyval() is added, as already proposed/hinted at
  in the rbd QAPI discussion.  This includes movement of many
  block-specific QDict function declarations into an own header, as
  suggested by Markus.

- 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(), qdict_stringify_for_keyval(), qdict_crumple(), the
  keyval 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

Max Reitz (13):
  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: Add block-specific QDict header
  qdict: Add qdict_stringify_for_keyval()
  tests: Add qdict_stringify_for_keyval() test
  qdict: Make qdict_flatten() shallow-clone-friendly
  tests: Add QDict clone-flatten test
  block: Try to create well typed json:{} filenames
  iotests: Test internal option typing
  iotests: qcow2's encrypt.format is now optional

 docs/devel/qapi-code-gen.txt                       | 21 +++++-
 tests/Makefile.include                             |  3 +
 qapi/block-core.json                               | 72 ++++++++++++++++--
 qapi/introspect.json                               |  8 ++
 ...ion-optional-discriminator-invalid-default.json | 12 +++
 ...at-union-optional-discriminator-no-default.json | 11 +++
 .../flat-union-optional-discriminator.json         |  4 +-
 .../flat-union-superfluous-default-variant.json    | 11 +++
 include/block/qdict.h                              | 37 +++++++++
 include/qapi/qmp/qdict.h                           | 17 -----
 block.c                                            | 70 ++++++++++++++++-
 block/gluster.c                                    |  1 +
 block/iscsi.c                                      |  1 +
 block/nbd.c                                        |  1 +
 block/nfs.c                                        |  1 +
 block/parallels.c                                  |  1 +
 block/qcow.c                                       |  1 +
 block/qcow2.c                                      |  1 +
 block/qed.c                                        |  1 +
 block/quorum.c                                     |  1 +
 block/rbd.c                                        |  1 +
 block/sheepdog.c                                   |  1 +
 block/snapshot.c                                   |  1 +
 block/ssh.c                                        |  1 +
 block/vhdx.c                                       |  1 +
 block/vpc.c                                        |  1 +
 block/vvfat.c                                      |  1 +
 block/vxhs.c                                       |  1 +
 blockdev.c                                         |  1 +
 qobject/qdict.c                                    | 81 ++++++++++++++++++--
 tests/check-qdict.c                                | 88 ++++++++++++++++++++++
 tests/check-qobject.c                              |  1 +
 tests/test-replication.c                           |  1 +
 util/qemu-config.c                                 |  1 +
 scripts/qapi/common.py                             | 57 +++++++++++---
 scripts/qapi/doc.py                                |  8 +-
 scripts/qapi/introspect.py                         | 10 ++-
 scripts/qapi/visit.py                              | 33 +++++++-
 ...nion-optional-discriminator-invalid-default.err |  1 +
 ...ion-optional-discriminator-invalid-default.exit |  1 +
 ...nion-optional-discriminator-invalid-default.out |  0
 ...lat-union-optional-discriminator-no-default.err |  1 +
 ...at-union-optional-discriminator-no-default.exit |  1 +
 ...lat-union-optional-discriminator-no-default.out |  0
 .../flat-union-optional-discriminator.err          |  1 -
 .../flat-union-optional-discriminator.exit         |  2 +-
 .../flat-union-optional-discriminator.out          | 15 ++++
 .../flat-union-superfluous-default-variant.err     |  1 +
 .../flat-union-superfluous-default-variant.exit    |  1 +
 .../flat-union-superfluous-default-variant.out     |  0
 tests/qapi-schema/test-qapi.py                     |  2 +
 tests/qemu-iotests/059.out                         |  2 +-
 tests/qemu-iotests/087                             |  2 -
 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 +--
 59 files changed, 580 insertions(+), 68 deletions(-)
 create mode 100644 
 create mode 100644 
 create mode 100644 
 create mode 100644 include/block/qdict.h
 create mode 100644 
 create mode 100644 
 create mode 100644 
 create mode 100644 
 create mode 100644 
 create mode 100644 
 create mode 100644 tests/qapi-schema/flat-union-superfluous-default-variant.err
 create mode 100644 
 create mode 100644 tests/qapi-schema/flat-union-superfluous-default-variant.out


reply via email to

[Prev in Thread] Current Thread [Next in Thread]