xfs
[Top] [All Lists]

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

To: Wanlong Gao <gaowanlong@xxxxxxxxxxxxxx>
Subject: Re: [PATCH V4] xfs: cleanup the mount options
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Wed, 11 Jul 2012 12:26:09 +1000
Cc: XFS <xfs@xxxxxxxxxxx>, Ben Myers <bpm@xxxxxxx>, Christoph Hellwig <hch@xxxxxxxxxxxxx>, Zach Brown <zab@xxxxxxxxx>
In-reply-to: <4FFAA2B4.6090503@xxxxxxxxxxxxxx>
References: <20120706030532.GU19223@dastard> <1341747397-10649-1-git-send-email-gaowanlong@xxxxxxxxxxxxxx> <20120709002218.GW19223@dastard> <4FFAA2B4.6090503@xxxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
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@xxxxxxx>
> >> CC: Christoph Hellwig <hch@xxxxxxxxxxxxx>
> >> CC: Dave Chinner <david@xxxxxxxxxxxxx>
> >> CC: Zach Brown <zab@xxxxxxxxx>
> >> Signed-off-by: Wanlong Gao <gaowanlong@xxxxxxxxxxxxxx>
> >> ---
> > 
> > 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@xxxxxxxxxxxxx

<Prev in Thread] Current Thread [Next in Thread>