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

[John Ferlan]

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?

Tks -

John