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

[Daniel P. Berrange]

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
> > 
> >  
> >demo.raw
> >5368709120
> >
> >  
> >  
> > > uuid='0a81f5b2-8403-7b23-c8d6-21ccd2f80d6f'/>
> >  
> >
> >  
> > 
> > and when configuring a guest disk we should use
> > 
> >  
> >
> >
> >
> >
> >  
> >
> >  
> > 
> > This commit thus removes the "luks" storage volume type added
> > in
> > 
> >  commit 318ebb36f1027b3357a32d6f781bd77d7a9043fd
> >  Author: John Ferlan 
> >  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 
> > ---
> > 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 +,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" 

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

[Martin Kletzander]

On Wed, Jul 27, 2016 at 10:23:18AM +0100, Daniel P. Berrange wrote:

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
> > >
> > >  
> > >demo.raw
> > >5368709120
> > >
> > >  
> > >  
> > >
> > >  
> > >
> > >  
> > >
> > > and when configuring a guest disk we should use
> > >
> > >  
> > >
> > >
> > >
> > >
> > >  
> > >
> > >  
> > >
> > > This commit thus removes the "luks" storage volume type added
> > > in
> > >
> > >  commit 318ebb36f1027b3357a32d6f781bd77d7a9043fd
> > >  Author: John Ferlan 
> > >  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 
> > > ---
> > > 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 +,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) {
> > > 

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

[Daniel P. Berrange]

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
> > > >
> > > >  
> > > >demo.raw
> > > >5368709120
> > > >
> > > >  
> > > >  
> > > > > > > uuid='0a81f5b2-8403-7b23-c8d6-21ccd2f80d6f'/>
> > > >  
> > > >
> > > >  
> > > >
> > > > and when configuring a guest disk we should use
> > > >
> > > >  
> > > >
> > > >
> > > >
> > > >
> > > >   > > > uuid='0a81f5b2-8403-7b23-c8d6-21ccd2f80d6f'/>
> > > >
> > > >  
> > > >
> > > > This commit thus removes the "luks" storage volume type added
> > > > in
> > > >
> > > >  commit 318ebb36f1027b3357a32d6f781bd77d7a9043fd
> > > >  Author: John Ferlan 
> > > >  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 
> > > > ---
> > > > 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 +,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 

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

[Martin Kletzander]

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
>
>  
>demo.raw
>5368709120
>
>  
>  
>
>  
>
>  
>
> and when configuring a guest disk we should use
>
>  
>
>
>
>
>  
>
>  
>
> This commit thus removes the "luks" storage volume type added
> in
>
>  commit 318ebb36f1027b3357a32d6f781bd77d7a9043fd
>  Author: John Ferlan 
>  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 
> ---
> 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 +,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
> --- 

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

[Daniel P. Berrange]

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
> > 
> >  
> >demo.raw
> >5368709120
> >
> >  
> >  
> > > uuid='0a81f5b2-8403-7b23-c8d6-21ccd2f80d6f'/>
> >  
> >
> >  
> > 
> > and when configuring a guest disk we should use
> > 
> >  
> >
> >
> >
> >
> >  
> >
> >  
> > 
> > This commit thus removes the "luks" storage volume type added
> > in
> > 
> >  commit 318ebb36f1027b3357a32d6f781bd77d7a9043fd
> >  Author: John Ferlan 
> >  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 
> > ---
> > 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 +,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 

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

[Martin Kletzander]

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

 
   demo.raw
   5368709120
   
 
 
   
 
   
 

and when configuring a guest disk we should use

 
   
   
   
   
 
   
 

This commit thus removes the "luks" storage volume type added
in

 commit 318ebb36f1027b3357a32d6f781bd77d7a9043fd
 Author: John Ferlan 
 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 
---
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 +,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)

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

[Daniel P. Berrange]

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

  
demo.raw
5368709120

  
  

  

  

and when configuring a guest disk we should use

  




  

  

This commit thus removes the "luks" storage volume type added
in

  commit 318ebb36f1027b3357a32d6f781bd77d7a9043fd
  Author: John Ferlan 
  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 
---
 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/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 4558b9f..d31fde3 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1232,9 +1232,13 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
  encinfo->s.aes.alias);
 
 if (disk->src->format > 0 &&
-disk->src->type != VIR_STORAGE_TYPE_DIR)
-virBufferAsprintf(, "format=%s,",
-  
virStorageFileFormatTypeToString(disk->src->format));
+disk->src->type != VIR_STORAGE_TYPE_DIR) {
+const char *qemuformat = 
virStorageFileFormatTypeToString(disk->src->format);
+if (disk->src->encryption &&
+disk->src->encryption->format == 
VIR_STORAGE_ENCRYPTION_FORMAT_LUKS)
+qemuformat = "luks";
+virBufferAsprintf(, "format=%s,", qemuformat);
+}
 }
 VIR_FREE(source);
 
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 9045f37..b3ca993 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -1078,7 +1078,7 @@ qemuDomainSecretDiskPrepare(virConnectPtr conn,
 }
 
 if (!virStorageSourceIsEmpty(src) && src->encryption &&
-src->format == VIR_STORAGE_FILE_LUKS) {
+src->encryption->format == VIR_STORAGE_ENCRYPTION_FORMAT_LUKS) {
 
 if (VIR_ALLOC(secinfo) < 0)
 return -1;
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 34a1424..b970448 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -2989,7 +2989,7 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
  * can remove the luks object password too
  */
 if (!virStorageSourceIsEmpty(disk->src) && disk->src->encryption &&
-disk->src->format == VIR_STORAGE_FILE_LUKS) {
+disk->src->encryption->format == VIR_STORAGE_ENCRYPTION_FORMAT_LUKS) {
 
 if (!(encAlias =
   qemuDomainGetSecretAESAlias(disk->info.alias, true))) {
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index