The destination disk's xml contains the ciper info and it check fail, when
snaphot,blockcopy and blockpull vm's disk. So it removes the check about
cipher info.

Signed-off-by: luzhipeng <luzhipeng@cestc.cn>
---
 src/conf/domain_validate.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
index 39b8d67928..b70edcaaa0 100644
--- a/src/conf/domain_validate.c
+++ b/src/conf/domain_validate.c
@@ -529,18 +529,6 @@ virDomainDiskDefValidateSourceChainOne(const virStorageSource *src)
         }
     }
 
-    if (src->encryption) {
-        virStorageEncryption *encryption = src->encryption;
-
-        if (encryption->format == VIR_STORAGE_ENCRYPTION_FORMAT_LUKS &&
-            encryption->encinfo.cipher_name) {
-
-            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-                           _("supplying <cipher> for domain disk definition is unnecessary"));
-            return -1;
-        }
-    }
-
     /* internal snapshots and config files are currently supported only with rbd: */
     if (virStorageSourceGetActualType(src) != VIR_STORAGE_TYPE_NETWORK &&
         src->protocol != VIR_STORAGE_NET_PROTOCOL_RBD) {
-- 
2.39.3

From subject:

The shortlog line doesn't really seem to describe what's happening.


On Mon, Aug 12, 2024 at 11:27:51 +0800, luzhipeng wrote:
> The destination disk's xml contains the ciper info and it check fail, when
> snaphot,blockcopy and blockpull vm's disk. So it removes the check about
> cipher info.

Is this part of a series with your next patch?

So IIUC you want to allow configuring the cipher for code paths which
create the image, right? Note that blockpull is not creating a new image
though, why is it on the list?

> 
> Signed-off-by: luzhipeng <luzhipeng@cestc.cn>
> ---
>  src/conf/domain_validate.c | 12 ------------
>  1 file changed, 12 deletions(-)
> 
> diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
> index 39b8d67928..b70edcaaa0 100644
> --- a/src/conf/domain_validate.c
> +++ b/src/conf/domain_validate.c
> @@ -529,18 +529,6 @@ virDomainDiskDefValidateSourceChainOne(const virStorageSource *src)
>          }
>      }
>  
> -    if (src->encryption) {
> -        virStorageEncryption *encryption = src->encryption;
> -
> -        if (encryption->format == VIR_STORAGE_ENCRYPTION_FORMAT_LUKS &&
> -            encryption->encinfo.cipher_name) {
> -
> -            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> -                           _("supplying <cipher> for domain disk definition is unnecessary"));
> -            return -1;

This check is designed to disallow the format of the image to be
configured on code paths where it would be ignored. That
is for paths which open an existing image.

So IIUC if you to create a copy or snapshot this would trip you up on
the next start of the VM, right? If that's the case then rather than
removing the check, the code which is adding/formatting the new image
should clear the cipher field once the image is formatted rather than
bypassing this check and then having users ask why we opened their image
even if the cipher was incorrect.

> -        }
> -    }
> -
>      /* internal snapshots and config files are currently supported only with rbd: */
>      if (virStorageSourceGetActualType(src) != VIR_STORAGE_TYPE_NETWORK &&
>          src->protocol != VIR_STORAGE_NET_PROTOCOL_RBD) {
> -- 
> 2.39.3
> 
> 

在 2024/8/12 16:08, Peter Krempa 写道:
>  From subject:
> 
> The shortlog line doesn't really seem to describe what's happening.
> 
> 
> On Mon, Aug 12, 2024 at 11:27:51 +0800, luzhipeng wrote:
>> The destination disk's xml contains the ciper info and it check fail, when
>> snaphot,blockcopy and blockpull vm's disk. So it removes the check about
>> cipher info.
> 
> Is this part of a series with your next patch?
The two patches are independent;
the next patch is  to fix inconsistent name in qemu and libvirt.
the qemu patch:
https://patchew.org/QEMU/20240730113850.30-1-luzhipeng@cestc.cn/

> 
> So IIUC you want to allow configuring the cipher for code paths which
> create the image, right? Note that blockpull is not creating a new image
> though, why is it on the list?
> 
yes, create the image with cipher info in xml by libvirt not open an 
existing image.

blockpull did not create new image, should not on the list.
>>
>> Signed-off-by: luzhipeng <luzhipeng@cestc.cn>
>> ---
>>   src/conf/domain_validate.c | 12 ------------
>>   1 file changed, 12 deletions(-)
>>
>> diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
>> index 39b8d67928..b70edcaaa0 100644
>> --- a/src/conf/domain_validate.c
>> +++ b/src/conf/domain_validate.c
>> @@ -529,18 +529,6 @@ virDomainDiskDefValidateSourceChainOne(const virStorageSource *src)
>>           }
>>       }
>>   
>> -    if (src->encryption) {
>> -        virStorageEncryption *encryption = src->encryption;
>> -
>> -        if (encryption->format == VIR_STORAGE_ENCRYPTION_FORMAT_LUKS &&
>> -            encryption->encinfo.cipher_name) {
>> -
>> -            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> -                           _("supplying <cipher> for domain disk definition is unnecessary"));
>> -            return -1;
> 
> This check is designed to disallow the format of the image to be
> configured on code paths where it would be ignored. That
> is for paths which open an existing image.
> 
> So IIUC if you to create a copy or snapshot this would trip you up on
> the next start of the VM, right? If that's the case then rather than
> removing the check, the code which is adding/formatting the new image
> should clear the cipher field once the image is formatted rather than
> bypassing this check and then having users ask why we opened their image
> even if the cipher was incorrect.
> 
yes, i will add some checks about  creating new image  or opening an 
existing image
>> -        }
>> -    }
>> -
>>       /* internal snapshots and config files are currently supported only with rbd: */
>>       if (virStorageSourceGetActualType(src) != VIR_STORAGE_TYPE_NETWORK &&
>>           src->protocol != VIR_STORAGE_NET_PROTOCOL_RBD) {
>> -- 
>> 2.39.3
>>
>>
> 
> 
>