xfs
[Top] [All Lists]

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

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH V5] xfs: cleanup the mount options
From: Wanlong Gao <gaowanlong@xxxxxxxxxxxxxx>
Date: Tue, 24 Jul 2012 10:01:59 +0800
Cc: xfs@xxxxxxxxxxx, Ben Myers <bpm@xxxxxxx>, Christoph Hellwig <hch@xxxxxxxxxxxxx>, Zach Brown <zab@xxxxxxxxx>
In-reply-to: <20120723232813.GL23387@dastard>
Organization: Fujitsu
References: <20120711022609.GX19223@dastard> <1341988145-12994-1-git-send-email-gaowanlong@xxxxxxxxxxxxxx> <20120723232813.GL23387@dastard>
Reply-to: gaowanlong@xxxxxxxxxxxxxx
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:14.0) Gecko/20120717 Thunderbird/14.0
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.
> 

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