Re: [libvirt] [PATCH 2/2] storage: remove "luks" storage volume type

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(a)redhat.com&gt; > > 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(a)redhat.com&gt; > > --- > > 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(a)redhat.com&gt; 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

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'

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 :|