Re: [PATCH v2] common/casefold: Support for tmpfs casefold test

["André Almeida" <andrealmeid@igalia.com>]

Em 04/11/2024 18:45, Gabriel Krisman Bertazi escreveu:
> André Almeida <andrealmeid@igalia.com> writes:
> 
> 
>> @@ -67,6 +74,10 @@ _scratch_mkfs_casefold_strict()
>>   	f2fs)
>>   		_scratch_mkfs -C utf8:strict
>>   		;;
>> +	tmpfs)
>> +		# there's no mkfs for tmpfs, so we just mount it
>> +		mount -t tmpfs -o casefold,strict_encoding tmpfs $SCRATCH_MNT
>> +		;;
> 
> 
> This looks like a hack.  Why mount it here instead of _scratch_mount?
> This should just be a nop for tmpfs.
> 
> 

_scratch_mount doesn't have any code related to casefold, should I 
create a _scratch_mount_casefold for tmpfs?

I added it here because is where the other filesystem do their casefold 
options setup.

>>   	*)
>>   		_notrun "Don't know how to mkfs with casefold-strict support on $FSTYP"
>>   		;;
>> diff --git a/common/rc b/common/rc
>> index 2af26f23..2ee46e51 100644
>> --- a/common/rc
>> +++ b/common/rc
>> @@ -374,6 +374,9 @@ _scratch_unmount()
>>   	btrfs)
>>   		$UMOUNT_PROG $SCRATCH_MNT
>>   		;;
>> +	tmpfs)
>> +		$UMOUNT_PROG $SCRATCH_MNT
>> +		;;
>>   	*)
>>   		$UMOUNT_PROG $SCRATCH_DEV
>>   		;;
>> diff --git a/tests/generic/556 b/tests/generic/556
>> index 51d2d482..78cdf1bf 100755
>> --- a/tests/generic/556
>> +++ b/tests/generic/556
>> @@ -485,11 +485,20 @@ test_strict_mode_invalid_filename()
>>   
>>   _scratch_mkfs_casefold >>$seqres.full 2>&1
>>   
>> -_scratch_mount
>> -
>> -_check_dmesg_for \
>> -	"\(${sdev}\): Using encoding defined by superblock: utf8" || \
>> -	_fail "Could not mount with encoding: utf8"
>> +# casefold tmpfs was already mounted in the mkfs step
>> +if [ "$FSTYP" != "tmpfs" ]; then
>> +	_scratch_mount
>> +fi
> 
>   can you fix _scratch_mount to correctly mount casefolded tmpfs?  if
> not, provide a new local handler that will either call _scratch_mount
> directly for !tmpfs or do the correct mount for tmpfs.
> 
>> +
>> +if [ "$FSTYP" == "tmpfs" ]; then
>> +	_check_dmesg_for \
>> +		"tmpfs: Using encoding" || \
>> +		_fail "Could not mount with encoding: utf8"
>> +else
>> +	_check_dmesg_for \
>> +		"\(${sdev}\): Using encoding defined by superblock: utf8" || \
>> +		_fail "Could not mount with encoding: utf8"
>> +fi
> 
> Ugh. the kernel message should have been the same. Much better
> than special casing the test.
> 

Well, to be accurate the encoding isn't defined by the superblock in the 
case of tmpfs, so the message would be imprecise.

But I think the if wasn't needed after all, the _check_dmesg_for grep 
should be able to do fine like this:

"\(${sdev}\): Using encoding" || \
	_fail "Could not mount with encoding: utf8"


>>   
>>   test_casefold_flag_basic
>>   test_casefold_lookup
>> @@ -517,7 +526,9 @@ _check_scratch_fs
>>   
>>   # Test Strict Mode
>>   _scratch_mkfs_casefold_strict >>$seqres.full 2>&1
>> -_scratch_mount
>> +if [ "$FSTYP" != "tmpfs" ]; then
>> +	_scratch_mount
>> +fi
>>   
>>   test_strict_mode_invalid_filename
>