Re: [Qemu-devel] [PATCH v5 02/18] block: add ability to set a prefix for opt names

[Eric Blake]

On 02/22/2017 09:49 AM, Daniel P. Berrange wrote:
> On Wed, Feb 22, 2017 at 04:18:33PM +0100, Kevin Wolf wrote:
>> Am 21.02.2017 um 12:54 hat Daniel P. Berrange geschrieben:
>>> When integrating the crypto support with qcow/qcow2, we don't
>>> want to use the bare LUKS option names "hash-alg", "key-secret",
>>> etc. We want to namespace them "luks-hash-alg", "luks-key-secret"
>>> so that they don't clash with any general qcow options at a later
>>> date.
>>
>> Or maybe "luks.key-secret", i.e. actually embed the LUKS options QAPI
>> type into the qcow2 one? In that case, maybe qdict_extract_subqdict()
>> can even be used before calling into this, so that we don't have to
>> write a QemuOpts version of the function.
> 
> Yeah, the use of '-' vs '.'  is a significant decision point from
> a QAPI modelling POV. Currently as you see from earlier patch, I just
> add 'luks-key-secret' straight into BlockdevOptionsQcow2. ie I duplicate
> fields from QCryptoBlockOptionsLUKS straight into BlockdevOptionsQcow2.
> Likewise for fields from QCryptoBlockOptionsQCow.
> 
> If we used '.', we could potentially have BlockdevOptionsQcow2 directly
> reference the QCryptoBlockOptionsLUKS & QCryptoBlockOptionsQCow structs.

Using '.' would mean a layer of {} nesting on the wire, maybe as in:

{ "driver": "qcow2", ..., "luks" : { "hash-alg": ... } }

but conceptually, I like that a bit better, as it consolidates all the
luks-related options in one place, and may indeed make it possible to
reuse the type rather than having two variants (one prefixed, one not,
depending on whether it is standalone or qcow2).

I'm also looking later in your series (13/18), where you have:


@@ -2344,7 +2348,8 @@
             '*l2-cache-size': 'int',
             '*refcount-cache-size': 'int',
             '*cache-clean-interval': 'int',
-            '*aes-key-secret': 'str' } }
+            '*aes-key-secret': 'str',
+            '*luks-key-secret': 'str' } }


Uggh - we have two optional parameters, that must not both be present at
once.  I'm wondering if we can instead do this (hmm, my patches for
anonymous base/branches in a flat union haven't been taken yet, but you
get the idea):

...
'*cache-clean-interval': 'int',
'*encryption': 'Qcow2Encryption' } }

{ 'enum': 'Qcow2EncryptionType': [ 'aes', 'luks' ] }
{ 'union': 'Qcow2Encryption', 'base': { 'type': 'Qcow2EncryptionType' },
  'discriminator': 'type', 'data': {
    'aes': { 'key-secret': 'str' },
    'luks': { 'key-secret': 'str', '*hash-alg': ..., '*slot': 'int' } } }

so that you can only provide one encryption type, but once you have that
type, you can then provide all the associated fields for that type.  So
the QMP would look like:

{ "driver": "qcow2", ..., "encryption" : { "type": "luks", "hash-alg":
... } }

> 
> As a point of reference though, we didn't do this for the standalone
> 'luks' block driver - BlockdevOptionsLUKS contains a copy of the
> fields from QCryptoBlockOptionsLUKS because it needs to add
> inheritance from BlockdevOptionsGenericFormat.
> 
> Both approaches have pluses & minuses and I don't have a particularly
> strong opinion either way, so interested to hear what others thing.

I think there are enough potentials for alternative designs, especially
where we can reuse code/structs without having to do prefixes, that it
is worth exploring some of those possibilities to see if they turn out
nicer.

> 
>> However, the only option I can see at the end of this series in
>> BlockdevOptionsQcow2 is luks-key-secret, so what happened with this
>> plan?
>>
>> And if we really have only luks-key-secret (and that not in a separate
>> sub-dict), I don't really see the need to have separate aes-key-secret
>> and luks-key-secret options.
> 
> Currently, all except the key-secret are create time options, not
> runtime and blockdev hasn't qapi'ified create time options. So the
> bits like luks-cipher-alg, luks-cipher-mode, etc don't appear in
> the QAPI schema, only the QemuOpts runtime_opts variable.
> 
> That both the legacy AES & LUKS modes only have a single runtime
> option is just a co-incidence that is likely to be a temporary
> state of affairs. It is quite possible that we'll gain more runtime
> LUKS options in the future. For example, options to specify which
> precise keyslot to load, or to provide the name of a separate image
> containing a standalone luks header, etc.

See my idea above about sticking a '*slot':'int' into the schema at the
right place; made easier if the qcow2-level member 'encryption' is a
nested flat union.

> 
> Hence, I wanted to have separation between the legacy AES & LUKS
> namespaces, to make it clear what applies to what scenario.

Again, I think the idea of a flat union, with the discriminator, makes
it easier to enforce mutual exclusion, rather than having two top-level
optional fields that cannot both be specified at once.  Maybe we should
also consider making qapi flat unions support a default discriminator,
so that the "type":"luks" can be omitted, but that's sugar.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature