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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx>
> >  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@xxxxxxxxxx>
> > ---
> > 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.

Previously it wasn't possible to reach this method when format == raw.
With LUKS now being treated as raw, we can reach this method in theory
and so we do still need to have this error check.

> 
> > @@ -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.

Yes, will make this more generic.


> > @@ -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?

Opps, yes.


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

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]