[libvirt] [PATCH 05/12] storage_util: Generate the qcow secret earlier

[Peter Krempa]

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 at 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 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180515/a38b9c05/attachment-0001.sig>