diff mbox series

[1/2] encrypt: rename _scratch_mkfs_encrypted to _scratch_check_encrypted

Message ID 20221026070418.259351-2-xiubli@redhat.com (mailing list archive)
State New, archived
Headers show
Series encrypt: add ceph support | expand

Commit Message

Xiubo Li Oct. 26, 2022, 7:04 a.m. UTC 
From: Xiubo Li <xiubli@redhat.com>

For ceph we couldn't check the encryption feature by mkfs, we need
to mount it first and then check the 'get_encpolicy' ioctl cmd.

Signed-off-by: Xiubo Li <xiubli@redhat.com>
---
 common/encrypt | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

Zorro Lang Oct. 26, 2022, 2:04 p.m. UTC | #1 
On Wed, Oct 26, 2022 at 03:04:17PM +0800, xiubli@redhat.com wrote:
> From: Xiubo Li <xiubli@redhat.com>
> 
> For ceph we couldn't check the encryption feature by mkfs, we need
> to mount it first and then check the 'get_encpolicy' ioctl cmd.
> 
> Signed-off-by: Xiubo Li <xiubli@redhat.com>
> ---

This patch only does an *incomplete* function rename, without any change
on that function body, that doesn't make sense, and even will bring in
regression, due to lots of cases depend on this common function. If you
really need to change a common function, please "grep" [1] this function
in xfstests-dev/ to find out and check all places use it at least.

The _scratch_mkfs_encrypted is a "mkfs" function, likes _scratch_mkfs. I
think we shouldn't change its name. If you want to check if encryption
feature is supported by ceph, I think you might hope to do that in
_require_* functions, or you even can have a ceph specific function in
common/ceph. Just not the way you did in this patchset.

Thanks,
Zorro

[1]
$ grep -rsn _scratch_mkfs_encrypted .
./common/encrypt:32:    if ! _scratch_mkfs_encrypted &>>$seqres.full; then
./common/encrypt:146:_scratch_mkfs_encrypted()
./common/encrypt:174:# Like _scratch_mkfs_encrypted(), but add -O stable_inodes if applicable for the
./common/encrypt:186:           _scratch_mkfs_encrypted
./common/encrypt:926:           _scratch_mkfs_encrypted &>> $seqres.full
./common/verity:178:_scratch_mkfs_encrypted_verity()
./common/verity:190:            _notrun "$FSTYP not supported in _scratch_mkfs_encrypted_verity"
./tests/ext4/024:36:_scratch_mkfs_encrypted &>>$seqres.full
./tests/generic/395:22:_scratch_mkfs_encrypted &>> $seqres.full
./tests/generic/396:21:_scratch_mkfs_encrypted &>> $seqres.full
./tests/generic/580:23:_scratch_mkfs_encrypted &>> $seqres.full
./tests/generic/581:36:_scratch_mkfs_encrypted &>> $seqres.full
./tests/generic/595:35:_scratch_mkfs_encrypted &>> $seqres.full
./tests/generic/613:29:_scratch_mkfs_encrypted &>> $seqres.full
./tests/generic/621:57:_scratch_mkfs_encrypted &>> $seqres.full
./tests/generic/429:36:_scratch_mkfs_encrypted &>> $seqres.full
./tests/generic/397:28:_scratch_mkfs_encrypted &>> $seqres.full
./tests/generic/398:28:_scratch_mkfs_encrypted &>> $seqres.full
./tests/generic/421:24:_scratch_mkfs_encrypted &>> $seqres.full
./tests/generic/440:29:_scratch_mkfs_encrypted &>> $seqres.full
./tests/generic/419:29:_scratch_mkfs_encrypted &>> $seqres.full
./tests/generic/435:33:_scratch_mkfs_encrypted &>> $seqres.full
./tests/generic/593:24:_scratch_mkfs_encrypted &>> $seqres.full
./tests/generic/576:34:_scratch_mkfs_encrypted_verity &>> $seqres.full



>  common/encrypt | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/common/encrypt b/common/encrypt
> index 45ce0954..fd620c41 100644
> --- a/common/encrypt
> +++ b/common/encrypt
> @@ -29,7 +29,7 @@ _require_scratch_encryption()
>  	# Make a filesystem on the scratch device with the encryption feature
>  	# enabled.  If this fails then probably the userspace tools (e.g.
>  	# e2fsprogs or f2fs-tools) are too old to understand encryption.
> -	if ! _scratch_mkfs_encrypted &>>$seqres.full; then
> +	if ! _scratch_check_encrypted &>>$seqres.full; then
>  		_notrun "$FSTYP userspace tools do not support encryption"
>  	fi
>  
> @@ -143,7 +143,7 @@ _require_encryption_policy_support()
>  	rm -r $dir
>  }
>  
> -_scratch_mkfs_encrypted()
> +_scratch_check_encrypted()
>  {
>  	case $FSTYP in
>  	ext4|f2fs)
> @@ -171,7 +171,7 @@ _scratch_mkfs_sized_encrypted()
>  	esac
>  }
>  
> -# Like _scratch_mkfs_encrypted(), but add -O stable_inodes if applicable for the
> +# Like _scratch_check_encrypted(), but add -O stable_inodes if applicable for the
>  # filesystem type.  This is necessary for using encryption policies that include
>  # the inode number in the IVs, e.g. policies with the IV_INO_LBLK_64 flag set.
>  _scratch_mkfs_stable_inodes_encrypted()
> @@ -183,7 +183,7 @@ _scratch_mkfs_stable_inodes_encrypted()
>  		fi
>  		;;
>  	*)
> -		_scratch_mkfs_encrypted
> +		_scratch_check_encrypted
>  		;;
>  	esac
>  }
> @@ -923,7 +923,7 @@ _verify_ciphertext_for_encryption_policy()
>  			      FSCRYPT_POLICY_FLAG_IV_INO_LBLK_32) )); then
>  		_scratch_mkfs_stable_inodes_encrypted &>> $seqres.full
>  	else
> -		_scratch_mkfs_encrypted &>> $seqres.full
> +		_scratch_check_encrypted &>> $seqres.full
>  	fi
>  	_scratch_mount
>  
> -- 
> 2.31.1
>
Xiubo Li Oct. 27, 2022, 2:19 a.m. UTC | #2 
On 26/10/2022 22:04, Zorro Lang wrote:
> On Wed, Oct 26, 2022 at 03:04:17PM +0800, xiubli@redhat.com wrote:
>> From: Xiubo Li <xiubli@redhat.com>
>>
>> For ceph we couldn't check the encryption feature by mkfs, we need
>> to mount it first and then check the 'get_encpolicy' ioctl cmd.
>>
>> Signed-off-by: Xiubo Li <xiubli@redhat.com>
>> ---
> This patch only does an *incomplete* function rename, without any change
> on that function body, that doesn't make sense, and even will bring in
> regression, due to lots of cases depend on this common function. If you
> really need to change a common function, please "grep" [1] this function
> in xfstests-dev/ to find out and check all places use it at least.
>
> The _scratch_mkfs_encrypted is a "mkfs" function, likes _scratch_mkfs. I
> think we shouldn't change its name. If you want to check if encryption
> feature is supported by ceph, I think you might hope to do that in
> _require_* functions, or you even can have a ceph specific function in
> common/ceph. Just not the way you did in this patchset.

Okay, make sense and will do that.

Thanks Zorro.

- Xiubo

>
> Thanks,
> Zorro
>
> [1]
> $ grep -rsn _scratch_mkfs_encrypted .
> ./common/encrypt:32:    if ! _scratch_mkfs_encrypted &>>$seqres.full; then
> ./common/encrypt:146:_scratch_mkfs_encrypted()
> ./common/encrypt:174:# Like _scratch_mkfs_encrypted(), but add -O stable_inodes if applicable for the
> ./common/encrypt:186:           _scratch_mkfs_encrypted
> ./common/encrypt:926:           _scratch_mkfs_encrypted &>> $seqres.full
> ./common/verity:178:_scratch_mkfs_encrypted_verity()
> ./common/verity:190:            _notrun "$FSTYP not supported in _scratch_mkfs_encrypted_verity"
> ./tests/ext4/024:36:_scratch_mkfs_encrypted &>>$seqres.full
> ./tests/generic/395:22:_scratch_mkfs_encrypted &>> $seqres.full
> ./tests/generic/396:21:_scratch_mkfs_encrypted &>> $seqres.full
> ./tests/generic/580:23:_scratch_mkfs_encrypted &>> $seqres.full
> ./tests/generic/581:36:_scratch_mkfs_encrypted &>> $seqres.full
> ./tests/generic/595:35:_scratch_mkfs_encrypted &>> $seqres.full
> ./tests/generic/613:29:_scratch_mkfs_encrypted &>> $seqres.full
> ./tests/generic/621:57:_scratch_mkfs_encrypted &>> $seqres.full
> ./tests/generic/429:36:_scratch_mkfs_encrypted &>> $seqres.full
> ./tests/generic/397:28:_scratch_mkfs_encrypted &>> $seqres.full
> ./tests/generic/398:28:_scratch_mkfs_encrypted &>> $seqres.full
> ./tests/generic/421:24:_scratch_mkfs_encrypted &>> $seqres.full
> ./tests/generic/440:29:_scratch_mkfs_encrypted &>> $seqres.full
> ./tests/generic/419:29:_scratch_mkfs_encrypted &>> $seqres.full
> ./tests/generic/435:33:_scratch_mkfs_encrypted &>> $seqres.full
> ./tests/generic/593:24:_scratch_mkfs_encrypted &>> $seqres.full
> ./tests/generic/576:34:_scratch_mkfs_encrypted_verity &>> $seqres.full
>
>
>
>>   common/encrypt | 10 +++++-----
>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/common/encrypt b/common/encrypt
>> index 45ce0954..fd620c41 100644
>> --- a/common/encrypt
>> +++ b/common/encrypt
>> @@ -29,7 +29,7 @@ _require_scratch_encryption()
>>   	# Make a filesystem on the scratch device with the encryption feature
>>   	# enabled.  If this fails then probably the userspace tools (e.g.
>>   	# e2fsprogs or f2fs-tools) are too old to understand encryption.
>> -	if ! _scratch_mkfs_encrypted &>>$seqres.full; then
>> +	if ! _scratch_check_encrypted &>>$seqres.full; then
>>   		_notrun "$FSTYP userspace tools do not support encryption"
>>   	fi
>>   
>> @@ -143,7 +143,7 @@ _require_encryption_policy_support()
>>   	rm -r $dir
>>   }
>>   
>> -_scratch_mkfs_encrypted()
>> +_scratch_check_encrypted()
>>   {
>>   	case $FSTYP in
>>   	ext4|f2fs)
>> @@ -171,7 +171,7 @@ _scratch_mkfs_sized_encrypted()
>>   	esac
>>   }
>>   
>> -# Like _scratch_mkfs_encrypted(), but add -O stable_inodes if applicable for the
>> +# Like _scratch_check_encrypted(), but add -O stable_inodes if applicable for the
>>   # filesystem type.  This is necessary for using encryption policies that include
>>   # the inode number in the IVs, e.g. policies with the IV_INO_LBLK_64 flag set.
>>   _scratch_mkfs_stable_inodes_encrypted()
>> @@ -183,7 +183,7 @@ _scratch_mkfs_stable_inodes_encrypted()
>>   		fi
>>   		;;
>>   	*)
>> -		_scratch_mkfs_encrypted
>> +		_scratch_check_encrypted
>>   		;;
>>   	esac
>>   }
>> @@ -923,7 +923,7 @@ _verify_ciphertext_for_encryption_policy()
>>   			      FSCRYPT_POLICY_FLAG_IV_INO_LBLK_32) )); then
>>   		_scratch_mkfs_stable_inodes_encrypted &>> $seqres.full
>>   	else
>> -		_scratch_mkfs_encrypted &>> $seqres.full
>> +		_scratch_check_encrypted &>> $seqres.full
>>   	fi
>>   	_scratch_mount
>>   
>> -- 
>> 2.31.1
>>
diff mbox series

Patch

diff --git a/common/encrypt b/common/encrypt
index 45ce0954..fd620c41 100644
--- a/common/encrypt
+++ b/common/encrypt
@@ -29,7 +29,7 @@  _require_scratch_encryption()
 	# Make a filesystem on the scratch device with the encryption feature
 	# enabled.  If this fails then probably the userspace tools (e.g.
 	# e2fsprogs or f2fs-tools) are too old to understand encryption.
-	if ! _scratch_mkfs_encrypted &>>$seqres.full; then
+	if ! _scratch_check_encrypted &>>$seqres.full; then
 		_notrun "$FSTYP userspace tools do not support encryption"
 	fi
 
@@ -143,7 +143,7 @@  _require_encryption_policy_support()
 	rm -r $dir
 }
 
-_scratch_mkfs_encrypted()
+_scratch_check_encrypted()
 {
 	case $FSTYP in
 	ext4|f2fs)
@@ -171,7 +171,7 @@  _scratch_mkfs_sized_encrypted()
 	esac
 }
 
-# Like _scratch_mkfs_encrypted(), but add -O stable_inodes if applicable for the
+# Like _scratch_check_encrypted(), but add -O stable_inodes if applicable for the
 # filesystem type.  This is necessary for using encryption policies that include
 # the inode number in the IVs, e.g. policies with the IV_INO_LBLK_64 flag set.
 _scratch_mkfs_stable_inodes_encrypted()
@@ -183,7 +183,7 @@  _scratch_mkfs_stable_inodes_encrypted()
 		fi
 		;;
 	*)
-		_scratch_mkfs_encrypted
+		_scratch_check_encrypted
 		;;
 	esac
 }
@@ -923,7 +923,7 @@  _verify_ciphertext_for_encryption_policy()
 			      FSCRYPT_POLICY_FLAG_IV_INO_LBLK_32) )); then
 		_scratch_mkfs_stable_inodes_encrypted &>> $seqres.full
 	else
-		_scratch_mkfs_encrypted &>> $seqres.full
+		_scratch_check_encrypted &>> $seqres.full
 	fi
 	_scratch_mount