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

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?