Re: [PATCH 2/2] btrfs: sysfs: use kstrtoull_suffix() to replace memparse()

[Qu Wenruo <wqu@suse.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 2757 bytes --]



On 2023/12/18 18:19, David Disseldorp wrote:
> Hi Qu,
> 
> On Fri, 15 Dec 2023 19:09:24 +1030, Qu Wenruo wrote:
> 
>> Since memparse() itself can not handle overflow at all, use
>> memparse_ull() to be extra safe.
> 
> s/memparse_ull/kstrtoull_suffix/
> 
>> Now overflow values can be properly detected.
> 
> Please document how the sysfs API changes with this, in addition to
> overflow handling:
> - support for 'E' / 'e' suffixes dropped
> - only one trailing '\n' accepted, instead of many isspace()

Well, multiple spaces are already an abuse, and I don't believe sane 
scripts should go multiple spaces/newlines.

As all the other call sites are going kstrtox, which only accept one 
newline.

Although the change is indeed worthy a document update.

Thanks,
Qu
> 
> The latter might break a few scripts.
> 
> Cheers, David
> 
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   fs/btrfs/sysfs.c | 20 ++++++++------------
>>   1 file changed, 8 insertions(+), 12 deletions(-)
>>
>> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
>> index 84c05246ffd8..089c3fc123fe 100644
>> --- a/fs/btrfs/sysfs.c
>> +++ b/fs/btrfs/sysfs.c
>> @@ -760,7 +760,7 @@ static ssize_t btrfs_chunk_size_store(struct kobject *kobj,
>>   {
>>   	struct btrfs_space_info *space_info = to_space_info(kobj);
>>   	struct btrfs_fs_info *fs_info = to_fs_info(get_btrfs_kobj(kobj));
>> -	char *retptr;
>> +	int ret;
>>   	u64 val;
>>   
>>   	if (!capable(CAP_SYS_ADMIN))
>> @@ -776,11 +776,9 @@ static ssize_t btrfs_chunk_size_store(struct kobject *kobj,
>>   	if (space_info->flags & BTRFS_BLOCK_GROUP_SYSTEM)
>>   		return -EPERM;
>>   
>> -	val = memparse(buf, &retptr);
>> -	/* There could be trailing '\n', also catch any typos after the value */
>> -	retptr = skip_spaces(retptr);
>> -	if (*retptr != 0 || val == 0)
>> -		return -EINVAL;
>> +	ret = kstrtoull_suffix(buf, 0, &val, KSTRTOULL_SUFFIX_DEFAULT);
>> +	if (ret < 0)
>> +		return ret;
>>   
>>   	val = min(val, BTRFS_MAX_DATA_CHUNK_SIZE);
>>   
>> @@ -1779,14 +1777,12 @@ static ssize_t btrfs_devinfo_scrub_speed_max_store(struct kobject *kobj,
>>   {
>>   	struct btrfs_device *device = container_of(kobj, struct btrfs_device,
>>   						   devid_kobj);
>> -	char *endptr;
>>   	unsigned long long limit;
>> +	int ret;
>>   
>> -	limit = memparse(buf, &endptr);
>> -	/* There could be trailing '\n', also catch any typos after the value. */
>> -	endptr = skip_spaces(endptr);
>> -	if (*endptr != 0)
>> -		return -EINVAL;
>> +	ret = kstrtoull_suffix(buf, 0, &limit, KSTRTOULL_SUFFIX_DEFAULT);
>> +	if (ret < 0)
>> +		return ret;
>>   	WRITE_ONCE(device->scrub_speed_max, limit);
>>   	return len;
>>   }
> 
> 
> 
> 

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 7027 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]