Re: [PATCH 1/2] encrypt: rename _scratch_mkfs_encrypted to _scratch_check_encrypted

From: Xiubo Li <xiubli@redhat.com>
To: Zorro Lang <zlang@redhat.com>
Cc: fstests@vger.kernel.org, david@fromorbit.com, djwong@kernel.org,
	lhenriques@suse.de, ceph-devel@vger.kernel.org,
	jlayton@kernel.org, mchangir@redhat.com
Subject: Re: [PATCH 1/2] encrypt: rename _scratch_mkfs_encrypted to _scratch_check_encrypted
Date: Thu, 27 Oct 2022 10:19:45 +0800	[thread overview]
Message-ID: <c2796b1a-ed80-2951-c690-74a05aefbb3e@redhat.com> (raw)
In-Reply-To: <20221026140444.6br63mundxivfsnn@zlang-mailbox>

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
>>