[libvirt] [PATCH 2/2] storage: remove "luks" storage volume type
Martin Kletzander
mkletzan at redhat.com
Wed Jul 27 09:16:57 UTC 2016
On Wed, Jul 27, 2016 at 09:29:29AM +0100, Daniel P. Berrange wrote:
>On Wed, Jul 27, 2016 at 08:44:53AM +0200, Martin Kletzander wrote:
>> On Tue, Jul 26, 2016 at 07:12:30PM +0100, Daniel P. Berrange wrote:
>> > The current LUKS support has a "luks" volume type which has
>> > a "luks" encryption format.
>> >
>> > This partially makes sense if you consider the QEMU shorthand
>> > syntax only requires you to specify a format=luks, and it'll
>> > automagically uses "raw" as the next level driver. QEMU will
>> > however let you override the "raw" with any other driver it
>> > supports (vmdk, qcow, rbd, iscsi, etc, etc)
>> >
>> > IOW the intention though is that the "luks" encryption format
>> > is applied to all disk formats (whether raw, qcow2, rbd, gluster
>> > or whatever). As such it doesn't make much sense for libvirt
>> > to say the volume type is "luks" - we should be saying that it
>> > is a "raw" file, but with "luks" encryption applied.
>> >
>> > IOW, when creating a storage volume we should use this XML
>> >
>> > <volume>
>> > <name>demo.raw</name>
>> > <capacity>5368709120</capacity>
>> > <target>
>> > <format type='raw'/>
>> > <encryption format='luks'>
>> > <secret type='passphrase' uuid='0a81f5b2-8403-7b23-c8d6-21ccd2f80d6f'/>
>> > </encryption>
>> > </target>
>> > </volume>
>> >
>> > and when configuring a guest disk we should use
>> >
>> > <disk type='file' device='disk'>
>> > <driver name='qemu' type='raw'/>
>> > <source file='/home/berrange/VirtualMachines/demo.raw'/>
>> > <target dev='sda' bus='scsi'/>
>> > <encryption format='luks'>
>> > <secret type='passphrase' uuid='0a81f5b2-8403-7b23-c8d6-21ccd2f80d6f'/>
>> > </encryption>
>> > </disk>
>> >
>> > This commit thus removes the "luks" storage volume type added
>> > in
>> >
>> > commit 318ebb36f1027b3357a32d6f781bd77d7a9043fd
>> > Author: John Ferlan <jferlan at redhat.com>
>> > Date: Tue Jun 21 12:59:54 2016 -0400
>> >
>> > util: Add 'luks' to the FileTypeInfo
>> >
>> > The storage file probing code is modified so that it can probe
>> > the actual encryption formats explicitly, rather than merely
>> > probing existance of encryption and letting the storage driver
>> > guess the format.
>> >
>> > The rest of the code is then adapted to deal with
>> > VIR_STORAGE_FILE_RAW w/ VIR_STORAGE_ENCRYPTION_FORMAT_LUKS
>> > instead of just VIR_STORAGE_FILE_LUKS.
>> >
>> > The commit mentioned above was included in libvirt v2.0.0.
>> > So when querying volume XML this will be a change in behaviour
>> > vs the 2.0.0 release - it'll report 'raw' instead of 'luks'
>> > for the volume format, but still report 'luks' for encryption
>> > format. I think this change is OK because the storage driver
>> > did not include any support for creating volumes, nor starting
>> > guets with luks volumes in v2.0.0 - that only since then.
>> > Clearly if we change this we must do it before v2.1.0 though.
>> >
>> > Signed-off-by: Daniel P. Berrange <berrange at redhat.com>
>> > ---
>> > src/qemu/qemu_command.c | 10 +-
>> > src/qemu/qemu_domain.c | 2 +-
>> > src/qemu/qemu_hotplug.c | 2 +-
>> > src/storage/storage_backend.c | 41 +++--
>> > src/storage/storage_backend_fs.c | 17 +-
>> > src/storage/storage_backend_gluster.c | 5 -
>> > src/util/virstoragefile.c | 202 ++++++++++++++++-----
>> > src/util/virstoragefile.h | 1 -
>> > tests/qemuxml2argvdata/qemuxml2argv-luks-disks.xml | 4 +-
>> > tests/storagevolxml2xmlin/vol-luks-cipher.xml | 2 +-
>> > tests/storagevolxml2xmlin/vol-luks.xml | 2 +-
>> > tests/storagevolxml2xmlout/vol-luks-cipher.xml | 2 +-
>> > tests/storagevolxml2xmlout/vol-luks.xml | 2 +-
>> > 13 files changed, 198 insertions(+), 94 deletions(-)
>> >
>> > diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
>> > index 862fb29..5ef536a 100644
>> > --- a/src/storage/storage_backend.c
>> > +++ b/src/storage/storage_backend.c
>> > @@ -1116,9 +1111,9 @@ virStorageBackendCreateQemuImgSetBacking(virStoragePoolObjPtr pool,
>> > int accessRetCode = -1;
>> > char *absolutePath = NULL;
>> >
>> > - if (info->format == VIR_STORAGE_FILE_LUKS) {
>> > + if (info->format == VIR_STORAGE_FILE_RAW) {
>> > virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> > - _("cannot set backing store for luks volume"));
>> > + _("cannot set backing store for raw volume"));
>> > return -1;
>> > }
>> >
>>
>> I think this whole condition can be removed as it wasn't there before
>> luks volumes.
>>
>> > @@ -1283,7 +1278,8 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn,
>> > _("format features only available with qcow2"));
>> > return NULL;
>> > }
>> > - if (info.format == VIR_STORAGE_FILE_LUKS) {
>> > + if (info.format == VIR_STORAGE_FILE_RAW &&
>> > + vol->target.encryption != NULL) {
>> > if (inputvol) {
>> > virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> > _("cannot use inputvol with luks volume"));
>>
>> You're still reporting the error for "luks volume" here.
>>
>> > @@ -1484,13 +1490,16 @@ virStorageBackendGetBuildVolFromFunction(virStorageVolDefPtr vol,
>> > if (!inputvol)
>> > return NULL;
>> >
>> > - /* If either volume is a non-raw file vol, we need to use an external
>> > - * tool for converting
>> > + VIR_WARN("BUild vol from func");
>>
>> Leftover from debugging?
>>
>> > diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
>> > index 2834baa..c264041 100644
>> > --- a/src/util/virstoragefile.c
>> > +++ b/src/util/virstoragefile.c
>> > @@ -808,6 +882,45 @@ qcow2GetFeatures(virBitmapPtr *features,
>> > }
>> >
>> >
>> > +static bool
>> > +virStorageFileHasEncryptionFormat(const struct FileEncryptionInfo *info,
>> > + char *buf,
>> > + size_t len)
>> > +{
>> > + if (!info->magic && info->modeOffset == -1)
>> > + return 0; /* Shouldn't happen - expect at least one */
>> > +
>> > + if (info->magic) {
>> > + if (!virStorageFileMatchesMagic(info->magicOffset,
>> > + info->magic,
>> > + buf, len))
>> > + return false;
>> > +
>> > + if (info->versionOffset != -1 &&
>> > + !virStorageFileMatchesVersion(info->versionOffset,
>> > + info->versionSize,
>> > + info->versionNumbers,
>> > + info->endian,
>> > + buf, len))
>> > + return false;
>> > +
>> > + return true;
>> > + } else if (info->modeOffset != -1) {
>> > + if (info->modeOffset >= len)
>> > + return false;
>> > +
>> > + if (buf[info->modeOffset] != info->modeValue)
>> > + return false;
>> > +
>> > + return true;
>> > + } else {
>> > + return false;
>> > + }
>> > +}
>> > +
>> > +
>> > +
>> > +
>>
>> Getting used to 2 empty lines took me some time, but we're going four
>> now? Should I look for campaigns proclaiming "Four is the new Two!"? =)
>>
>> > @@ -820,6 +933,7 @@ virStorageFileGetMetadataInternal(virStorageSourcePtr meta,
>> > int *backingFormat)
>> > {
>> > int ret = -1;
>> > + size_t j;
>> >
>>
>> Why 'j'?
>>
>> Apart from those first three nits pointed out the patch is fine. I also
>> like the fact that it automatically get's rid of a problem with
>> format='luks' and no encryption specified (where we just waited for QEMU
>> to fail at startup). The problem is that, as you said, it was released
>> in v2.0.0 and you can have a domain with such disk. I think we need to
>> make a workaround where "luks" is some kind of alias to "raw" (but also
>> make sure it will require encryption because you could have luks without
>> encryption secret). At least we don't have that in the documentation
>> (even though it should've been).
>
>No, you can't have a domain with type="luks" AFAICT. QEMU LUKS support
>was added in:
>
> commit da86c6c22674ccc147224afa2740e33d8cbdbf22
> Author: John Ferlan <jferlan at redhat.com>
> Date: Thu Jun 2 16:28:28 2016 -0400
>
> qemu: Add luks support for domain disk
>
>Which is not yet released as its post-2.0.0:
>
> $ git describe da86c6c22674ccc147224afa2740e33d8cbdbf22
> v2.0.0-204-gda86c6c
>
This only added the support in qemu driver, but it can be defined
whenever it's added to the enum.
>
>AFAIK, all that v2.0.0 did was allow 'vol-dumpxml' to show "luks" for
>pre-existing files formatted with luks outside of libvirt. I think
>the number of people using that will be approximately zero, and even
>then I think its reasonable to argue that behaviour was a regression
>from pre-2.0.0, since people's 'raw' disks suddenly changed to 'luks'
>
That commit was post-v2.0.0, so that means we don't have a release in
which you could be able to start such VM. Unfortunately we have such
one that allows you to define the domain. I'm not sure how I feel about
such things. I tried forcing the back-compat in the past so that we
don't lose domains even though they were not sensibly configured at all,
but OTOH I understand it adds a lot of code that's just mess. Maybe we
could have a better policy for it. And mention it somewhere becuase
this is not the first time I'm having such discussion. Or someone could
come up with a way how to solve some of the issues in my patchset for
parsing invalid domains that would deal with not only this issue.
>
>Regards,
>Daniel
>--
>|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
>|: http://libvirt.org -o- http://virt-manager.org :|
>|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
>|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20160727/ee69974a/attachment-0001.sig>
More information about the libvir-list
mailing list