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@xxxxxxxxxx> > >> --- > >> 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
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list