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. 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. > 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. Hence, I wanted to have separation between the legacy AES & LUKS namespaces, to make it clear what applies to what scenario. > > Reviewed-by: Max Reitz <mre...@redhat.com> > > Reviewed-by: Alberto Garcia <be...@igalia.com> > > Signed-off-by: Daniel P. Berrange <berra...@redhat.com> > > Having said all that, while I'm not sure if the goal of the patch is > completely right, it does seem to correctly implement what it promises. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
