Rather than having storageBackendCreateQemuImgCheckEncryption
perform the virStorageGenerateQcowEncryption, let's just do that
earlier during storageBackendCreateQemuImg so that the check
helper is just a check helper rather doing something different
based on whether the format is qcow[2] or raw based encryption.
Signed-off-by: John Ferlan <jferlan@redhat.com>
---
src/storage/storage_util.c | 31 +++++++++++++++++++++++++++----
1 file changed, 27 insertions(+), 4 deletions(-)
diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c
index 37a649d17b..64d4d1d7d2 100644
--- a/src/storage/storage_util.c
+++ b/src/storage/storage_util.c
@@ -901,10 +901,10 @@ storageBackendCreateQemuImgCheckEncryption(int format,
_("too many secrets for qcow encryption"));
return -1;
}
- if (enc->format == VIR_STORAGE_ENCRYPTION_FORMAT_DEFAULT ||
- enc->nsecrets == 0) {
- if (virStorageGenerateQcowEncryption(vol) < 0)
- return -1;
+ if (enc->nsecrets == 0) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("no secret provided for qcow encryption"));
+ return -1;
}
} else if (format == VIR_STORAGE_FILE_RAW) {
if (enc->format != VIR_STORAGE_ENCRYPTION_FORMAT_LUKS) {
@@ -1309,6 +1309,26 @@ storageBackendCreateQemuImgSecretPath(virStoragePoolObjPtr pool,
}
+static int
+storageBackendGenerateSecretData(virStorageVolDefPtr vol)
+{
+ virStorageEncryptionPtr enc = vol->target.encryption;
+
+ if (!enc)
+ return 0;
+
+ if ((vol->target.format == VIR_STORAGE_FILE_QCOW ||
+ vol->target.format == VIR_STORAGE_FILE_QCOW2) &&
+ (enc->format == VIR_STORAGE_ENCRYPTION_FORMAT_DEFAULT ||
+ enc->nsecrets == 0)) {
+ if (virStorageGenerateQcowEncryption(vol) < 0)
+ return -1;
+ }
+
+ return 0;
+}
+
+
static int
storageBackendCreateQemuImg(virStoragePoolObjPtr pool,
virStorageVolDefPtr vol,
@@ -1330,6 +1350,9 @@ storageBackendCreateQemuImg(virStoragePoolObjPtr pool,
return -1;
}
+ if (storageBackendGenerateSecretData(vol) < 0)
+ goto cleanup;
+
if (vol->target.format == VIR_STORAGE_FILE_RAW &&
vol->target.encryption &&
vol->target.encryption->format == VIR_STORAGE_ENCRYPTION_FORMAT_LUKS) {
--
2.14.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, May 08, 2018 at 08:47:58 -0400, John Ferlan wrote:
> Rather than having storageBackendCreateQemuImgCheckEncryption
> perform the virStorageGenerateQcowEncryption, let's just do that
> earlier during storageBackendCreateQemuImg so that the check
> helper is just a check helper rather doing something different
> based on whether the format is qcow[2] or raw based encryption.
>
> Signed-off-by: John Ferlan <jferlan@redhat.com>
> ---
> src/storage/storage_util.c | 31 +++++++++++++++++++++++++++----
> 1 file changed, 27 insertions(+), 4 deletions(-)
>
> diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c
> index 37a649d17b..64d4d1d7d2 100644
> --- a/src/storage/storage_util.c
> +++ b/src/storage/storage_util.c
> @@ -901,10 +901,10 @@ storageBackendCreateQemuImgCheckEncryption(int format,
> _("too many secrets for qcow encryption"));
> return -1;
> }
> - if (enc->format == VIR_STORAGE_ENCRYPTION_FORMAT_DEFAULT ||
> - enc->nsecrets == 0) {
> - if (virStorageGenerateQcowEncryption(vol) < 0)
> - return -1;
> + if (enc->nsecrets == 0) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("no secret provided for qcow encryption"));
> + return -1;
> }
> } else if (format == VIR_STORAGE_FILE_RAW) {
> if (enc->format != VIR_STORAGE_ENCRYPTION_FORMAT_LUKS) {
> @@ -1309,6 +1309,26 @@ storageBackendCreateQemuImgSecretPath(virStoragePoolObjPtr pool,
storageBackendCreateQemuImgCheckEncryption is called from three
externally accessible call chains paths:
1) via multiple apis and then storageBackendCreateQemuImg
This one is fixed below.
2) via testCompareXMLToArgvFiles->virStorageBackendCreateQemuImgCmdFromVol
This may not be necessary to be fixed.
3) via virStorageBackendVolResizeLocal->storageBackendResizeQemuImg
This one looks like a regression.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 05/15/2018 10:12 AM, Peter Krempa wrote:
> On Tue, May 08, 2018 at 08:47:58 -0400, John Ferlan wrote:
>> Rather than having storageBackendCreateQemuImgCheckEncryption
>> perform the virStorageGenerateQcowEncryption, let's just do that
>> earlier during storageBackendCreateQemuImg so that the check
>> helper is just a check helper rather doing something different
>> based on whether the format is qcow[2] or raw based encryption.
>>
>> Signed-off-by: John Ferlan <jferlan@redhat.com>
>> ---
>> src/storage/storage_util.c | 31 +++++++++++++++++++++++++++----
>> 1 file changed, 27 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c
>> index 37a649d17b..64d4d1d7d2 100644
>> --- a/src/storage/storage_util.c
>> +++ b/src/storage/storage_util.c
>> @@ -901,10 +901,10 @@ storageBackendCreateQemuImgCheckEncryption(int format,
>> _("too many secrets for qcow encryption"));
>> return -1;
>> }
>> - if (enc->format == VIR_STORAGE_ENCRYPTION_FORMAT_DEFAULT ||
>> - enc->nsecrets == 0) {
>> - if (virStorageGenerateQcowEncryption(vol) < 0)
>> - return -1;
>> + if (enc->nsecrets == 0) {
>> + virReportError(VIR_ERR_XML_ERROR, "%s",
>> + _("no secret provided for qcow encryption"));
>> + return -1;
>> }
>> } else if (format == VIR_STORAGE_FILE_RAW) {
>> if (enc->format != VIR_STORAGE_ENCRYPTION_FORMAT_LUKS) {
>> @@ -1309,6 +1309,26 @@ storageBackendCreateQemuImgSecretPath(virStoragePoolObjPtr pool,
>
> storageBackendCreateQemuImgCheckEncryption is called from three
> externally accessible call chains paths:
>
> 1) via multiple apis and then storageBackendCreateQemuImg
>
> This one is fixed below.
>
>
> 2) via testCompareXMLToArgvFiles->virStorageBackendCreateQemuImgCmdFromVol
>
> This may not be necessary to be fixed.
>
>
> 3) via virStorageBackendVolResizeLocal->storageBackendResizeQemuImg
>
> This one looks like a regression.
>
[turned off wrapping to avoid nasty looking cut-n-paste from code]
Hmmm... let's see...
storageBackendResizeQemuImg()
{
...
if (vol->target.encryption) {
...
storageBackendLoadDefaultSecrets(vol);
if (storageBackendCreateQemuImgCheckEncryption(vol->target.format,
type, vol) < 0)
goto cleanup;
...
Leading us to:
storageBackendLoadDefaultSecrets()
{
...
if (!vol->target.encryption || vol->target.encryption->nsecrets != 0)
return 0;
...
otherwise we fill in nsecrets/secrets with the secret for the volume (if
found), meaning when we leave we'd have nsecrets == 1. Because nsecrets == 1
that means the CheckEncryption will not attempt to create a secret.
If a secret for the volume is not found, then yes we leave with nsecrets == 0
and seemingly would/could have a regression.
But let's consider the ramifications and that we created the volume with
a specific secret, but we could not find that secret later on when someone
went to resize the volume.
Currently if this were a luks volume, then the resize would fail in the
CheckEncryption because there is no secret. However, for a qcow volume
we'd create a new secret!
With the new code we'd generate the same failure that luks has but with
a qcow specific error message instead of regenerating a new secret for
resize that wasn't used for create.
So is the new model a regression or a fix?
Tks -
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, May 15, 2018 at 11:42:26 -0400, John Ferlan wrote:
>
>
> On 05/15/2018 10:12 AM, Peter Krempa wrote:
> > On Tue, May 08, 2018 at 08:47:58 -0400, John Ferlan wrote:
> >> Rather than having storageBackendCreateQemuImgCheckEncryption
> >> perform the virStorageGenerateQcowEncryption, let's just do that
> >> earlier during storageBackendCreateQemuImg so that the check
> >> helper is just a check helper rather doing something different
> >> based on whether the format is qcow[2] or raw based encryption.
> >>
> >> Signed-off-by: John Ferlan <jferlan@redhat.com>
> >> ---
> >> src/storage/storage_util.c | 31 +++++++++++++++++++++++++++----
> >> 1 file changed, 27 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c
> >> index 37a649d17b..64d4d1d7d2 100644
> >> --- a/src/storage/storage_util.c
> >> +++ b/src/storage/storage_util.c
> >> @@ -901,10 +901,10 @@ storageBackendCreateQemuImgCheckEncryption(int format,
> >> _("too many secrets for qcow encryption"));
> >> return -1;
> >> }
> >> - if (enc->format == VIR_STORAGE_ENCRYPTION_FORMAT_DEFAULT ||
> >> - enc->nsecrets == 0) {
> >> - if (virStorageGenerateQcowEncryption(vol) < 0)
> >> - return -1;
> >> + if (enc->nsecrets == 0) {
> >> + virReportError(VIR_ERR_XML_ERROR, "%s",
> >> + _("no secret provided for qcow encryption"));
> >> + return -1;
> >> }
> >> } else if (format == VIR_STORAGE_FILE_RAW) {
> >> if (enc->format != VIR_STORAGE_ENCRYPTION_FORMAT_LUKS) {
> >> @@ -1309,6 +1309,26 @@ storageBackendCreateQemuImgSecretPath(virStoragePoolObjPtr pool,
> >
> > storageBackendCreateQemuImgCheckEncryption is called from three
> > externally accessible call chains paths:
> >
> > 1) via multiple apis and then storageBackendCreateQemuImg
> >
> > This one is fixed below.
> >
> >
> > 2) via testCompareXMLToArgvFiles->virStorageBackendCreateQemuImgCmdFromVol
> >
> > This may not be necessary to be fixed.
> >
> >
> > 3) via virStorageBackendVolResizeLocal->storageBackendResizeQemuImg
> >
> > This one looks like a regression.
> >
>
> [turned off wrapping to avoid nasty looking cut-n-paste from code]
>
> Hmmm... let's see...
>
> storageBackendResizeQemuImg()
> {
> ...
> if (vol->target.encryption) {
> ...
> storageBackendLoadDefaultSecrets(vol);
>
> if (storageBackendCreateQemuImgCheckEncryption(vol->target.format,
> type, vol) < 0)
> goto cleanup;
> ...
>
> Leading us to:
>
> storageBackendLoadDefaultSecrets()
> {
> ...
> if (!vol->target.encryption || vol->target.encryption->nsecrets != 0)
> return 0;
> ...
>
>
> otherwise we fill in nsecrets/secrets with the secret for the volume (if
> found), meaning when we leave we'd have nsecrets == 1. Because nsecrets == 1
> that means the CheckEncryption will not attempt to create a secret.
>
> If a secret for the volume is not found, then yes we leave with nsecrets == 0
> and seemingly would/could have a regression.
>
> But let's consider the ramifications and that we created the volume with
> a specific secret, but we could not find that secret later on when someone
> went to resize the volume.
>
> Currently if this were a luks volume, then the resize would fail in the
> CheckEncryption because there is no secret. However, for a qcow volume
> we'd create a new secret!
>
> With the new code we'd generate the same failure that luks has but with
> a qcow specific error message instead of regenerating a new secret for
> resize that wasn't used for create.
>
> So is the new model a regression or a fix?
Oh! Yes it is actually a fix. I did not read virStorageGenerateQcowEncryption
closely enough to notice that it actually adds a new secret.
That even explains the rather weird logic used when calling this
function.
ACK to this patch
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.