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

[Dave Chinner <david@fromorbit.com>]

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.

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


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

....

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

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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