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

On Wed, Jul 27, 2016 at 11:16:57AM +0200, Martin Kletzander wrote: > 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(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 > > > > 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. Personally I'm not fussed about compat for something where you could not actually use the thing you defined, because it means its highly unlikely anyone would have bothered to define the thing in the first place, or if they did they would have undefined it shortly afterwards on the basis that it didn't work.

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