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

[Dave Chinner <david@fromorbit.com>]

On Sun, Jul 08, 2012 at 07:36:37PM +0800, Wanlong Gao wrote:
> remove the mount options macro, use tokens instead.
> 
> CC: Ben Myers <bpm@sgi.com>
> CC: Christoph Hellwig <hch@infradead.org>
> CC: Dave Chinner <david@fromorbit.com>
> CC: Zach Brown <zab@zabbo.net>
> Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
> ---

A "what's changed in this version" list would be handy here.

>  fs/xfs/xfs_super.c |  539 +++++++++++++++++++++++++++++++---------------------
>  1 file changed, 320 insertions(+), 219 deletions(-)

....

> -
> -STATIC unsigned long
> -suffix_strtoul(char *s, char **endp, unsigned int base)
> +STATIC int
> +suffix_match_int(substring_t *s, int *result)

I'm not sure ints are the best unit to use here....

>  {
> -	int	last, shift_left_factor = 0;
> -	char	*value = s;
> +	int ret;
> +	int last, shift_left_factor = 0;
> +	char *value = s->to - 1;
>  
> -	last = strlen(value) - 1;
> -	if (value[last] == 'K' || value[last] == 'k') {
> +	if (*value == 'K' || *value == 'k') {
>  		shift_left_factor = 10;
> -		value[last] = '\0';
> +		s->to--;
>  	}
> -	if (value[last] == 'M' || value[last] == 'm') {
> +	if (*value == 'M' || *value == 'm') {
>  		shift_left_factor = 20;
> -		value[last] = '\0';
> +		s->to--;
>  	}
> -	if (value[last] == 'G' || value[last] == 'g') {
> +	if (*value == 'G' || *value == 'g') {
>  		shift_left_factor = 30;
> -		value[last] = '\0';
> +		s->to--;
>  	}
>  
> -	return simple_strtoul((const char *)s, endp, base) << shift_left_factor;
> +	ret = match_number(s, result, 0);
> +	*result = *result << shift_left_factor;

Because this overflows or gives the negative values for numbers like
2G far too easily. I think this function needs to return an unsigned
long.

> +	ret = ENOMEM;
> +	options = kstrdup(options, GFP_NOFS);
> +	if (!options)
> +		goto done;

I commented on this error form previously. Can you convert them all
to be consistent with the rest of the code? i.e:

	options = kstrdup(options, GFP_NOFS);
	if (!options) {
		ret = ENOMEM;
		goto done;
	}

i.e. set the error value (if necessary) in the error branch....

> +	orig = options;
> +
> +	while ((p = strsep(&orig, ",")) != NULL) {
> +		int token;
> +		if (!*p)
>  			continue;
> +
> +		token = match_token(p, tokens, args);
> +		switch (token) {
> +		case Opt_logbufs:
> +			intarg = 0;

Please move the initialisation of intarg up above the switch
statement so that it is initialised once for all cases instead of
individually in the cases that use it. That means we don't have to
remember to do it in future for new or changed mount options...

> +			ret = match_int(args, &intarg);
> +			if (ret)
> +				goto free_orig;
> +			mp->m_logbufs = intarg;
> +			break;
> +		case Opt_logbsize:
> +			ret = suffix_match_int(args, &intarg);
> +			if (ret)
> +				goto free_orig;
> +			mp->m_logbsize = intarg;
> +			break;
> +		case Opt_logdev:
> +			ret = ENOMEM;
> +			string = match_strdup(args);
> +			if (!string)
> +				goto free_orig;
> +
> +			mp->m_logname = kstrndup(string, MAXNAMELEN, GFP_KERNEL);
>  			if (!mp->m_logname)
> +				goto free_string;

This match_strdup/kstrndup pattern is repeated a couple of times,
and requires a special failure case (goto free_string) - wrapping it
in helper function is probably a good idea so that the special
failure case can be removed from this main parse loop...

> +		case Opt_biosize:
> +			intarg = 0;
> +			ret = match_int(args, &intarg);
> +			if (ret)
> +				goto free_orig;
> +			iosizelog = ffs(intarg) - 1;
> +			break;
> +		case Opt_allocsize:
> +			ret = suffix_match_int(args, &intarg);
> +			if (ret)
> +				goto free_orig;
> +			iosizelog = ffs(intarg) - 1;
> +			break;

These two can be collapsed into:

		case Opt_biosize:
		case Opt_allocsize:
			ret = suffix_match_int(args, &intarg);
			if (ret)
				goto free_orig;
			iosizelog = ffs(intarg) - 1;
			break;

Also, these two a a good example of why intarg should be initialised
to zero outside the switch statement - they both do almost exactly
the same thing, but one initialises intarg and the other doesn't. Is
that a bug? I can't tell without looking at the implementations of
match_int and suffix_match_int....

> +		case Opt_delaylog:
> +			xfs_warn(mp,
> +		"delaylog is the default now, option is deprecated.");

As preivously mentioned, duplication of the mount option is here.

Adding something like:

#define MNTOPT_DEPRECATED "has no effect, option is deprecated"

Means these can become:

		case Opt_delaylog:
			xfs_warn(mp, MNTOPT_DELAYLOG MNTOPT_DEPRECATED);
			break;

> +		case Opt_nodelaylog:
> +			xfs_warn(mp,
> +		"nodelaylog support has been removed, option is deprecated.");

			xfs_warn(mp, MNTOPT_NODELAYLOG MNTOPT_DEPRECATED);

> +			break;
> +		case Opt_ihashsize:
>  			xfs_warn(mp,
> +		"ihashsize no longer used, option is deprecated.");

			xfs_warn(mp, MNTOPT_IHASHSIZE MNTOPT_DEPRECATED);

And so on for all the deprecated options. That way we get a
consistent mesage for all deprecated options and it's easy to keep
that way in the future.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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