xfs
[Top] [All Lists]

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

To: Wanlong Gao <gaowanlong@xxxxxxxxxxxxxx>
Subject: Re: [PATCH V5] xfs: cleanup the mount options
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Tue, 24 Jul 2012 09:28:13 +1000
Cc: xfs@xxxxxxxxxxx, Ben Myers <bpm@xxxxxxx>, Christoph Hellwig <hch@xxxxxxxxxxxxx>, Zach Brown <zab@xxxxxxxxx>
In-reply-to: <1341988145-12994-1-git-send-email-gaowanlong@xxxxxxxxxxxxxx>
References: <20120711022609.GX19223@dastard> <1341988145-12994-1-git-send-email-gaowanlong@xxxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
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@xxxxxxxxxxxxx

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