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