Re: [PATCH V5] xfs: cleanup the mount options

[Wanlong Gao <gaowanlong@cn.fujitsu.com>]

On 07/24/2012 07:28 AM, Dave Chinner wrote:
> On Wed, Jul 11, 2012 at 02:29:05PM +0800, Wanlong Gao wrote:
>> -	return simple_strtoul((const char *)s, endp, base) << shift_left_factor;
>> +	string = match_strdup(s);
> 
> GFP_KERNEL allocation....
> 
>> +	if (!string)
>> +		return ENOMEM;
>> +
>> +	*result = simple_strtoul((const char *)string, NULL, 0) <<
>> +			shift_left_factor;
>> +
>> +	kfree(string);
>> +	return 0;
>> +}
>> +
>> +STATIC int
>> +match_name_strdup(substring_t *s, char *name)
>> +{
>> +	char *string;
>> +	string = match_strdup(s);
> 
> GFP_KERNEL allocation....
> 
>> +	if (!string)
>> +		return ENOMEM;
>> +
>> +	name = kstrndup(string, MAXNAMELEN, GFP_KERNEL);
> 
> GFP_KERNEL allocation....
> 
>> +	if (!name)
>> +		goto free;
>> +	return 0;
> 
> Leaks string - it should always be freed.

Sorry, will update.

> 
>> +free:
>> +	kfree(string);
>> +	return ENOMEM;
>>  }
>>  
>>  /*
>> @@ -166,11 +246,15 @@ xfs_parseargs(
>>  	char			*options)
>>  {
>>  	struct super_block	*sb = mp->m_super;
>> -	char			*this_char, *value, *eov;
>> +	char			*p;
>>  	int			dsunit = 0;
>>  	int			dswidth = 0;
>> -	int			iosize = 0;
>>  	__uint8_t		iosizelog = 0;
>> +	int			intarg;
>> +	unsigned long		ularg;
>> +	substring_t		args[MAX_OPT_ARGS];
>> +	char			*orig = NULL;
>> +	int			ret = 0;
>>  
>>  	/*
>>  	 * set up the mount name first so all the errors will refer to the
>> @@ -208,175 +292,192 @@ xfs_parseargs(
>>  	if (!options)
>>  		goto done;
>>  
>> -	while ((this_char = strsep(&options, ",")) != NULL) {
>> -		if (!*this_char)
>> +	options = kstrdup(options, GFP_NOFS);
> 
> GFP_NOFS allocation. Why is this GFP_NOFS, and all the other
> allocations GFP_KERNEL? If it is not safe to use GFP_KERNEL
> allocations here, then all of the above allocations need to be
> GFP_NOFS, too.

Since strsep() will change the options, so we should make GFP_NOFS
safely to dup the orig options, but the parse functions can safely
use GFP_KERNEL.

> 
> 
>> +	if (!options) {
>> +		ret = ENOMEM;
>> +		goto done;
>> +	}
>> +
>> +	orig = options;
> 
> Also, no need to set up orig like this. Just a simple:
> 
> 	orig = kstrdup(options, GFP_NOFS);
> 	if (!orig) {
> 		ret = ENOMEM;
> 		goto done;
> 	}
> 
> will work fine.

Yeah, thank you.

> 
> ....
> 
>>  	if (mp->m_logbufs > 0)
>> -		seq_printf(m, "," MNTOPT_LOGBUFS "=%d", mp->m_logbufs);
>> +		seq_printf(seq, "," MNTOPT_LOGBUFS "=%d", mp->m_logbufs);
>>  	if (mp->m_logbsize > 0)
>> -		seq_printf(m, "," MNTOPT_LOGBSIZE "=%dk", mp->m_logbsize >> 10);
>> +		seq_printf(seq, "," MNTOPT_LOGBSIZE "=%dk",
>> +				mp->m_logbsize >> 10);
> 
> Change of user visible output format here. That will break any
> scripts that are parsing the output and expecting numbers. Just
> leave it as a raw number.

OK, defer to you.

Thanks,
Wanlong Gao

> 
> Cheers,
> 
> Dave.
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs