src/storage/storage_backend_disk.c | 43 ++++++++++++++++++++++++-------------- 1 file changed, 27 insertions(+), 16 deletions(-)
https://bugzilla.redhat.com/show_bug.cgi?id=1560946
Following the model of the Logical backend, use qemu-img on
the created device to set up for LUKS encryption.
Signed-off-by: John Ferlan <jferlan@redhat.com>
---
works much better with the settle patch applied from:
https://www.redhat.com/archives/libvir-list/2018-May/msg01847.html
src/storage/storage_backend_disk.c | 43 ++++++++++++++++++++++++--------------
1 file changed, 27 insertions(+), 16 deletions(-)
diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c
index 7b4549c34d..a3003fd0b5 100644
--- a/src/storage/storage_backend_disk.c
+++ b/src/storage/storage_backend_disk.c
@@ -870,19 +870,13 @@ virStorageBackendDiskCreateVol(virStoragePoolObjPtr pool,
char *partFormat = NULL;
unsigned long long startOffset = 0, endOffset = 0;
virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
+ virErrorPtr save_err;
virCommandPtr cmd = virCommandNewArgList(PARTED,
def->source.devices[0].path,
"mkpart",
"--script",
NULL);
- if (vol->target.encryption != NULL) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- "%s", _("storage pool does not support encrypted "
- "volumes"));
- goto cleanup;
- }
-
if (virStorageBackendDiskPartFormat(pool, vol, &partFormat) != 0)
goto cleanup;
virCommandAddArg(cmd, partFormat);
@@ -893,6 +887,12 @@ virStorageBackendDiskCreateVol(virStoragePoolObjPtr pool,
goto cleanup;
}
+ /* If we're going to encrypt using LUKS, then we could need up to
+ * an extra 2MB for the LUKS header - so account for that now */
+ if (vol->target.encryption &&
+ vol->target.encryption->format == VIR_STORAGE_ENCRYPTION_FORMAT_LUKS)
+ endOffset += 2 * 1024 * 1024;
+
virCommandAddArgFormat(cmd, "%lluB", startOffset);
virCommandAddArgFormat(cmd, "%lluB", endOffset);
@@ -910,15 +910,15 @@ virStorageBackendDiskCreateVol(virStoragePoolObjPtr pool,
VIR_FREE(vol->target.path);
/* Fetch actual extent info, generate key */
- if (virStorageBackendDiskReadPartitions(pool, vol) < 0) {
- /* Best effort to remove the partition. Ignore any errors
- * since we could be calling this with vol->target.path == NULL
- */
- virErrorPtr save_err = virSaveLastError();
- ignore_value(virStorageBackendDiskDeleteVol(pool, vol, 0));
- virSetError(save_err);
- virFreeError(save_err);
- goto cleanup;
+ if (virStorageBackendDiskReadPartitions(pool, vol) < 0)
+ goto error;
+
+ if (vol->target.encryption) {
+ /* Adjust the sizes to account for the LUKS header */
+ vol->target.capacity -= 2 * 1024 * 1024;
+ vol->target.allocation -= 2 * 1024 * 1024;
+ if (virStorageBackendCreateVolUsingQemuImg(pool, vol, NULL, 0) < 0)
+ goto error;
}
res = 0;
@@ -927,8 +927,19 @@ virStorageBackendDiskCreateVol(virStoragePoolObjPtr pool,
VIR_FREE(partFormat);
virCommandFree(cmd);
return res;
+
+ error:
+ /* Best effort to remove the partition. Ignore any errors
+ * since we could be calling this with vol->target.path == NULL
+ */
+ save_err = virSaveLastError();
+ ignore_value(virStorageBackendDiskDeleteVol(pool, vol, 0));
+ virSetError(save_err);
+ virFreeError(save_err);
+ goto cleanup;
}
+
static int
virStorageBackendDiskBuildVolFrom(virStoragePoolObjPtr pool,
virStorageVolDefPtr vol,
--
2.14.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
ping?
Tks,
John
On 05/24/2018 07:50 PM, John Ferlan wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1560946
>
> Following the model of the Logical backend, use qemu-img on
> the created device to set up for LUKS encryption.
>
> Signed-off-by: John Ferlan <jferlan@redhat.com>
> ---
>
> works much better with the settle patch applied from:
>
> https://www.redhat.com/archives/libvir-list/2018-May/msg01847.html
>
>
> src/storage/storage_backend_disk.c | 43 ++++++++++++++++++++++++--------------
> 1 file changed, 27 insertions(+), 16 deletions(-)
>
> diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c
> index 7b4549c34d..a3003fd0b5 100644
> --- a/src/storage/storage_backend_disk.c
> +++ b/src/storage/storage_backend_disk.c
> @@ -870,19 +870,13 @@ virStorageBackendDiskCreateVol(virStoragePoolObjPtr pool,
> char *partFormat = NULL;
> unsigned long long startOffset = 0, endOffset = 0;
> virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
> + virErrorPtr save_err;
> virCommandPtr cmd = virCommandNewArgList(PARTED,
> def->source.devices[0].path,
> "mkpart",
> "--script",
> NULL);
>
> - if (vol->target.encryption != NULL) {
> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> - "%s", _("storage pool does not support encrypted "
> - "volumes"));
> - goto cleanup;
> - }
> -
> if (virStorageBackendDiskPartFormat(pool, vol, &partFormat) != 0)
> goto cleanup;
> virCommandAddArg(cmd, partFormat);
> @@ -893,6 +887,12 @@ virStorageBackendDiskCreateVol(virStoragePoolObjPtr pool,
> goto cleanup;
> }
>
> + /* If we're going to encrypt using LUKS, then we could need up to
> + * an extra 2MB for the LUKS header - so account for that now */
> + if (vol->target.encryption &&
> + vol->target.encryption->format == VIR_STORAGE_ENCRYPTION_FORMAT_LUKS)
> + endOffset += 2 * 1024 * 1024;
> +
> virCommandAddArgFormat(cmd, "%lluB", startOffset);
> virCommandAddArgFormat(cmd, "%lluB", endOffset);
>
> @@ -910,15 +910,15 @@ virStorageBackendDiskCreateVol(virStoragePoolObjPtr pool,
> VIR_FREE(vol->target.path);
>
> /* Fetch actual extent info, generate key */
> - if (virStorageBackendDiskReadPartitions(pool, vol) < 0) {
> - /* Best effort to remove the partition. Ignore any errors
> - * since we could be calling this with vol->target.path == NULL
> - */
> - virErrorPtr save_err = virSaveLastError();
> - ignore_value(virStorageBackendDiskDeleteVol(pool, vol, 0));
> - virSetError(save_err);
> - virFreeError(save_err);
> - goto cleanup;
> + if (virStorageBackendDiskReadPartitions(pool, vol) < 0)
> + goto error;
> +
> + if (vol->target.encryption) {
> + /* Adjust the sizes to account for the LUKS header */
> + vol->target.capacity -= 2 * 1024 * 1024;
> + vol->target.allocation -= 2 * 1024 * 1024;
> + if (virStorageBackendCreateVolUsingQemuImg(pool, vol, NULL, 0) < 0)
> + goto error;
> }
>
> res = 0;
> @@ -927,8 +927,19 @@ virStorageBackendDiskCreateVol(virStoragePoolObjPtr pool,
> VIR_FREE(partFormat);
> virCommandFree(cmd);
> return res;
> +
> + error:
> + /* Best effort to remove the partition. Ignore any errors
> + * since we could be calling this with vol->target.path == NULL
> + */
> + save_err = virSaveLastError();
> + ignore_value(virStorageBackendDiskDeleteVol(pool, vol, 0));
> + virSetError(save_err);
> + virFreeError(save_err);
> + goto cleanup;
> }
>
> +
> static int
> virStorageBackendDiskBuildVolFrom(virStoragePoolObjPtr pool,
> virStorageVolDefPtr vol,
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, May 24, 2018 at 19:50:09 -0400, John Ferlan wrote: > https://bugzilla.redhat.com/show_bug.cgi?id=1560946 > > Following the model of the Logical backend, use qemu-img on > the created device to set up for LUKS encryption. > > Signed-off-by: John Ferlan <jferlan@redhat.com> > --- > > works much better with the settle patch applied from: > > https://www.redhat.com/archives/libvir-list/2018-May/msg01847.html > > > src/storage/storage_backend_disk.c | 43 ++++++++++++++++++++++++-------------- > 1 file changed, 27 insertions(+), 16 deletions(-) > > diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c > index 7b4549c34d..a3003fd0b5 100644 > --- a/src/storage/storage_backend_disk.c > +++ b/src/storage/storage_backend_disk.c I must say that I'm not a fan of adding features to the 'disk' backend. Using the disk backend is borderline insane for managing disk partitions. [...] > @@ -893,6 +887,12 @@ virStorageBackendDiskCreateVol(virStoragePoolObjPtr pool, > goto cleanup; > } > > + /* If we're going to encrypt using LUKS, then we could need up to > + * an extra 2MB for the LUKS header - so account for that now */ > + if (vol->target.encryption && > + vol->target.encryption->format == VIR_STORAGE_ENCRYPTION_FORMAT_LUKS) > + endOffset += 2 * 1024 * 1024; I don't think it's a good idea to change 'endOffset' after calling virStorageBackendDiskPartBoundaries as the function is looking up space in the existing partition table. With this if the size is just right and you increase it afterwards the partition will not fit in the place found by that function. > + > virCommandAddArgFormat(cmd, "%lluB", startOffset); > virCommandAddArgFormat(cmd, "%lluB", endOffset); > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 05/29/2018 07:02 AM, Peter Krempa wrote: > On Thu, May 24, 2018 at 19:50:09 -0400, John Ferlan wrote: >> https://bugzilla.redhat.com/show_bug.cgi?id=1560946 >> >> Following the model of the Logical backend, use qemu-img on >> the created device to set up for LUKS encryption. >> >> Signed-off-by: John Ferlan <jferlan@redhat.com> >> --- >> >> works much better with the settle patch applied from: >> >> https://www.redhat.com/archives/libvir-list/2018-May/msg01847.html >> >> >> src/storage/storage_backend_disk.c | 43 ++++++++++++++++++++++++-------------- >> 1 file changed, 27 insertions(+), 16 deletions(-) >> >> diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c >> index 7b4549c34d..a3003fd0b5 100644 >> --- a/src/storage/storage_backend_disk.c >> +++ b/src/storage/storage_backend_disk.c > > I must say that I'm not a fan of adding features to the 'disk' backend. > Using the disk backend is borderline insane for managing disk > partitions. > > [...] > So in your opinion, does that mean the above bug should just closed? I don't want to assume anything about anyone's sanity ;-). Even though to a degree I agree as I found this particularly odd to create/pass a device/partition to qemu-img. In a way I was surprised it worked, but then again since the Logical back-end also allows encryption of a single volume - doing so for disk volumes would appear to be similar, albeit strange. Depending on configuration it may not stand the test of time (e.g. reboot) either especially if your infrastructure changes - you'd have to recreate the secret for a different /dev/sdXN. >> @@ -893,6 +887,12 @@ virStorageBackendDiskCreateVol(virStoragePoolObjPtr pool, >> goto cleanup; >> } >> >> + /* If we're going to encrypt using LUKS, then we could need up to >> + * an extra 2MB for the LUKS header - so account for that now */ >> + if (vol->target.encryption && >> + vol->target.encryption->format == VIR_STORAGE_ENCRYPTION_FORMAT_LUKS) >> + endOffset += 2 * 1024 * 1024; > > > I don't think it's a good idea to change 'endOffset' after calling > virStorageBackendDiskPartBoundaries as the function is looking up space > in the existing partition table. With this if the size is just right and > you increase it afterwards the partition will not fit in the place found > by that function. > Oh right, sigh. Brain cramp. If this were to be done, then need to adjust vol->target.capacity before virStorageBackendDiskPartBoundaries John >> + >> virCommandAddArgFormat(cmd, "%lluB", startOffset); >> virCommandAddArgFormat(cmd, "%lluB", endOffset); >> -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, May 29, 2018 at 07:49:50 -0400, John Ferlan wrote: > > > On 05/29/2018 07:02 AM, Peter Krempa wrote: > > On Thu, May 24, 2018 at 19:50:09 -0400, John Ferlan wrote: > >> https://bugzilla.redhat.com/show_bug.cgi?id=1560946 > >> > >> Following the model of the Logical backend, use qemu-img on > >> the created device to set up for LUKS encryption. > >> > >> Signed-off-by: John Ferlan <jferlan@redhat.com> > >> --- > >> > >> works much better with the settle patch applied from: > >> > >> https://www.redhat.com/archives/libvir-list/2018-May/msg01847.html > >> > >> > >> src/storage/storage_backend_disk.c | 43 ++++++++++++++++++++++++-------------- > >> 1 file changed, 27 insertions(+), 16 deletions(-) > >> > >> diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c > >> index 7b4549c34d..a3003fd0b5 100644 > >> --- a/src/storage/storage_backend_disk.c > >> +++ b/src/storage/storage_backend_disk.c > > > > I must say that I'm not a fan of adding features to the 'disk' backend. > > Using the disk backend is borderline insane for managing disk > > partitions. > > > > [...] > > > > So in your opinion, does that mean the above bug should just closed? I Well, in my opinion this feature will not be used much ... or perhaps at all. Whether it can be closed or not depends on the motivation of the ones filing it and specifically on the motivation of Red Hat keeping it on their product tracker. I just wanted to point that out. Obviously contributions to upstream are welcome so if a feature is desired and contributors implement it, it's marginal usefulnes for most cases should not interfere with it's upstream acceptance as it might be useful for somebody. > don't want to assume anything about anyone's sanity ;-). Even though to Well, my comment is specifically targetted on using libvirt to manage partitions. Using libvirt XML to describe partitioning changes is a very strange concept and users of it must be very brave. > a degree I agree as I found this particularly odd to create/pass a > device/partition to qemu-img. In a way I was surprised it worked, but Passing raw devices to qemu-img or qemu itself is done regularly. Even qcow2 formatted logical volumes are used e.g. by oVirt > then again since the Logical back-end also allows encryption of a single > volume - doing so for disk volumes would appear to be similar, albeit > strange. > > Depending on configuration it may not stand the test of time (e.g. > reboot) either especially if your infrastructure changes - you'd have to > recreate the secret for a different /dev/sdXN. Both paragraphs above can be applied to logical volumes too, there really isn't much difference. > > > >> @@ -893,6 +887,12 @@ virStorageBackendDiskCreateVol(virStoragePoolObjPtr pool, > >> goto cleanup; > >> } > >> > >> + /* If we're going to encrypt using LUKS, then we could need up to > >> + * an extra 2MB for the LUKS header - so account for that now */ > >> + if (vol->target.encryption && > >> + vol->target.encryption->format == VIR_STORAGE_ENCRYPTION_FORMAT_LUKS) > >> + endOffset += 2 * 1024 * 1024; > > > > > > I don't think it's a good idea to change 'endOffset' after calling > > virStorageBackendDiskPartBoundaries as the function is looking up space > > in the existing partition table. With this if the size is just right and > > you increase it afterwards the partition will not fit in the place found > > by that function. > > > > Oh right, sigh. Brain cramp. If this were to be done, then need to > adjust vol->target.capacity before virStorageBackendDiskPartBoundaries Yes. Specifically only the value passed to virStorageBackendDiskPartBoundaries since you want to keep the capacity desired by the user intact. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2024 Red Hat, Inc.