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

[Dave Chinner <david@fromorbit.com>]

On Mon, Jul 09, 2012 at 05:21:56PM +0800, Wanlong Gao wrote:
> On 07/09/2012 08:22 AM, Dave Chinner wrote:
> > 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.
> 
> Do you mean the "result" should be "unsigned long" but not the return value?
> Because the return value is a error state.

result.

BTW: *result <<= shift_left_factor;

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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