Re: [PATCH V3] lib/bitmap.c: rewrite __bitmap_parse && __bitmap_parselist

[Yury <yury.norov@gmail.com>]

 > Subject: lib/bitmap.c: rewrite __bitmap_parse && __bitmap_parselist
     scripts/checkpatch.pl 
lib_bitmap.c:-rewrite-__bitmap_parse-__bitmap_parselist.patch
     total: 134 errors, 1 warnings, 284 lines checked

     NOTE: whitespace errors detected, you may wish to use 
scripts/cleanpatch or
           scripts/cleanfile

Most of them are about DOS line endings, but it prevents me to apply 
your patch:
     patch -p1 < 
lib_bitmap.c:-rewrite-__bitmap_parse-__bitmap_parselist.patch
     (Stripping trailing CRs from patch; use --binary to disable.)
     patching file lib/bitmap.c
     Hunk #1 FAILED at 16.
     Hunk #2 FAILED at 331.
     Hunk #3 FAILED at 359.
     Hunk #4 FAILED at 417.
     Hunk #5 FAILED at 503.
     5 out of 5 hunks FAILED -- saving rejects to file lib/bitmap.c.rej
 >
 > add __bitmap_parse_common to match any contents and return expected 
reslut.
 >
 > as __bitmap_parse_common need NULL-terminated string, we alloc a new buf.
 >
 > this patch also fix an unexpected parse result issue in 
__bitmap_parselist.
 >
 > Signed-off-by: Pan Xinhui <xinhuix.pan@intel.com>
 > ---
 >   lib/bitmap.c | 238 
+++++++++++++++++++++++++++++++++--------------------------
 >   1 file changed, 134 insertions(+), 104 deletions(-)
 > ---
 > change log
 > v3:
 > __bitmap_parselist now allow some extra input, like ",2, 3,,,, 5-8", 
at least one digit inside.
 > input like " " ", " is still not allowed.
 >
 > V2:
 > __bitmap_parse_common need *base* to parse correct result
 >
 > V1:
 > add __bitmap_parse_common and rewrite __bitmap_parse && 
__bitmap_parselist
 > ---
 >
 > diff --git a/lib/bitmap.c b/lib/bitmap.c
 > index 64c0926..f2095e1d 100644
 > --- a/lib/bitmap.c
 > +++ b/lib/bitmap.c
 > @@ -16,6 +16,8 @@
 >   #include <asm/page.h>
 >   #include <asm/uaccess.h>
 >
 > +#include <linux/parser.h>
 > +#include <linux/slab.h>
 >   /*
 >    * bitmaps provide an array of bits, implemented using an an
 >    * array of unsigned longs.  The number of valid bits in a
 > @@ -331,6 +333,58 @@ again:
 >   EXPORT_SYMBOL(bitmap_find_next_zero_area_off);
 >
 >   /*
 > + * __bitmap_parse_common - parse expected number from buf
 > + * Return 0 on success.
 > + * there two patterns.
 > + * if buf's contents did not match any of them, reutrn equivalent error.
 > + * Notice buf's contents may be changed.
 > + */
 > +static int __bitmap_parse_common(char *buf, unsigned int buflen,
 > +         unsigned long *a, unsigned long *b, int base)

It looks weird, and I don't like in your version too much:
Name is bad.
There's nothing about bitmap. You just parsing a string for two 
patterns: %d, and %d-%d.

You do your work twice (at least): first you detect digits in 
match_token, then - in kstrtoul.
You allocate kbuf unconditionally, no matter you need it or not.
You do more than one thing in __bitmap_parse_common (you search number 
and region).
You modify initial string.

Let's consider a more straight interface:

     /*
      * I don't know why this function is not written yet.
      * Maybe it's something ideological...
      */
     void set_bits(unsigned long *bitmap, unsigned long start, unsigned 
long len);

     /*
      * Takes care of all user whitespaces and commas,
      * Return endp, or error if parse fails, or null if string reached 
the end.
      */
     char *parse_range(const char *buf, unsigned long *start, unsigned 
long *len);

than pattern usage would be:

     while (str = parse_range(str, &start, &len)) {
         if (IS_ERROR(str))
             return ...;
         if (start + len >= nbits)
             return ...;

         set_bits(bitmap, start, len);
     }

 > +{
 > +    int ret;
 > +    int token;
 > +    const match_table_t table = {
 > +        {
 > +            .token = 1,
 > +            .pattern = "%x",
 > +        },
 > +        {
 > +            .token = 2,
 > +            .pattern = "%x-%x",
 > +        },
 > +        {
 > +            .token = 0,
 > +            .pattern = NULL,
 > +        }
 > +    };
 > +    substring_t substr[MAX_OPT_ARGS];
 > +
 > +    if (!buflen || !a)
 > +        return -EINVAL;
 > +    token = match_token((char *)buf, table, substr);
 > +    switch (token) {
 > +    case 1:
 > +        *substr[0].to = '\0';
 > +        ret = kstrtoul(substr[0].from, base, a);
 > +        if (b)
 > +            *b = *a;
 > +        break;
 > +    case 2:
 > +        *substr[0].to = '\0';
 > +        *substr[1].to = '\0';
 > +        ret = kstrtoul(substr[0].from, base, a);
 > +        ret |= b ? kstrtoul(substr[1].from, base, b) : -EINVAL;
 > +        break;
 > +    default:
 > +        ret = -EINVAL;
 > +        break;
 > +    }
 > +    return ret;
 > +}
 > +
 > +/*
 >    * Bitmap printing & parsing functions: first version by Nadia 
Yvette Chambers,
 >    * second version by Paul Jackson, third by Joe Korty.
 >    */
 > @@ -359,57 +413,45 @@ int __bitmap_parse(const char *buf, unsigned 
int buflen,
 >           int is_user, unsigned long *maskp,
 >           int nmaskbits)
 >   {
 > -    int c, old_c, totaldigits, ndigits, nchunks, nbits;
 > +    int nchunks, nbits, ret;
 > +    unsigned long a;
 >       u32 chunk;
 >       const char __user __force *ubuf = (const char __user __force *)buf;
 > +    char *kbuf, *endp;
 > +
 > +    if (!buflen)
 > +        return -EINVAL;
 > +    kbuf = kmalloc(buflen + 1, GFP_KERNEL);
 > +    if (!kbuf)
 > +        return -ENOMEM;
 > +    if (is_user) {
 > +        if (copy_from_user(kbuf, ubuf, buflen) != 0) {
 > +            kfree(kbuf);
 > +            return -EFAULT;
 > +        }
 > +    } else
 > +        memcpy(kbuf, buf, buflen);
 > +    kbuf[buflen] = '\0';
 > +    buf = strim(kbuf);
 >
 >       bitmap_zero(maskp, nmaskbits);
 >
 > -    nchunks = nbits = totaldigits = c = 0;
 > +    nchunks = nbits = 0;
 >       do {
 > -        chunk = ndigits = 0;
 > -
 > -        /* Get the next chunk of the bitmap */
 > -        while (buflen) {
 > -            old_c = c;
 > -            if (is_user) {
 > -                if (__get_user(c, ubuf++))
 > -                    return -EFAULT;
 > -            }
 > -            else
 > -                c = *buf++;
 > -            buflen--;
 > -            if (isspace(c))
 > -                continue;
 > -
 > -            /*
 > -             * If the last character was a space and the current
 > -             * character isn't '\0', we've got embedded whitespace.
 > -             * This is a no-no, so throw an error.
 > -             */
 > -            if (totaldigits && c && isspace(old_c))
 > -                return -EINVAL;
 > -
 > -            /* A '\0' or a ',' signal the end of the chunk */
 > -            if (c == '\0' || c == ',')
 > -                break;
 > -
 > -            if (!isxdigit(c))
 > -                return -EINVAL;
 > -
 > -            /*
 > -             * Make sure there are at least 4 free bits in 'chunk'.
 > -             * If not, this hexdigit will overflow 'chunk', so
 > -             * throw an error.
 > -             */
 > -            if (chunk & ~((1UL << (CHUNKSZ - 4)) - 1))
 > -                return -EOVERFLOW;
 > -
 > -            chunk = (chunk << 4) | hex_to_bin(c);
 > -            ndigits++; totaldigits++;
 > +        endp = strchr(buf, ',');
 > +        if (endp)
 > +            *endp = '\0';
 > +        ret = __bitmap_parse_common((char *)buf, strlen(buf),
 > +                                &a, NULL, 16);
 > +        if (ret)
 > +            break;
 > +        buf = endp + 1;
 > +
 > +        if (unlikely(a > U32_MAX)) {
 > +            ret = -ERANGE;
 > +            break;
 >           }
 > -        if (ndigits == 0)
 > -            return -EINVAL;
 > +        chunk = (u32)a;
 >           if (nchunks == 0 && chunk == 0)
 >               continue;
 >
 > @@ -417,11 +459,13 @@ int __bitmap_parse(const char *buf, unsigned 
int buflen,
 >           *maskp |= chunk;
 >           nchunks++;
 >           nbits += (nchunks == 1) ? nbits_to_hold_value(chunk) : CHUNKSZ;
 > -        if (nbits > nmaskbits)
 > -            return -EOVERFLOW;
 > -    } while (buflen && c == ',');
 > -
 > -    return 0;
 > +        if (nbits > nmaskbits) {
 > +            ret = -EOVERFLOW;
 > +            break;
 > +        }
 > +    } while (endp);
 > +    kfree(kbuf);
 > +    return ret;
 >   }
 >   EXPORT_SYMBOL(__bitmap_parse);
 >
 > @@ -503,70 +547,56 @@ static int __bitmap_parselist(const char *buf, 
unsigned int buflen,
 >           int is_user, unsigned long *maskp,
 >           int nmaskbits)
 >   {
 > -    unsigned a, b;
 > -    int c, old_c, totaldigits;
 > +    unsigned long a, b;
 > +    int ret = -EINVAL;
 >       const char __user __force *ubuf = (const char __user __force *)buf;
 > -    int exp_digit, in_range;
 > +    char *kbuf, *endp, *ibuf;
 > +
 > +    if (!buflen)
 > +        return -EINVAL;
 > +    ibuf = kbuf = kmalloc(buflen + 1, GFP_KERNEL);
 > +    if (!kbuf)
 > +        return -ENOMEM;
 > +    if (is_user) {
 > +        if (copy_from_user(kbuf, ubuf, buflen) != 0) {
 > +            kfree(kbuf);
 > +            return -EFAULT;
 > +        }
 > +    } else
 > +        memcpy(kbuf, buf, buflen);
 > +    kbuf[buflen] = '\0';
 >
 > -    totaldigits = c = 0;
 >       bitmap_zero(maskp, nmaskbits);
 >       do {
 > -        exp_digit = 1;
 > -        in_range = 0;
 > -        a = b = 0;
 > -
 > -        /* Get the next cpu# or a range of cpu#'s */
 > -        while (buflen) {
 > -            old_c = c;
 > -            if (is_user) {
 > -                if (__get_user(c, ubuf++))
 > -                    return -EFAULT;
 > -            } else
 > -                c = *buf++;
 > -            buflen--;
 > -            if (isspace(c))
 > -                continue;
 > -
 > -            /*
 > -             * If the last character was a space and the current
 > -             * character isn't '\0', we've got embedded whitespace.
 > -             * This is a no-no, so throw an error.
 > -             */
 > -            if (totaldigits && c && isspace(old_c))
 > -                return -EINVAL;
 > -
 > -            /* A '\0' or a ',' signal the end of a cpu# or range */
 > -            if (c == '\0' || c == ',')
 > -                break;
 > -
 > -            if (c == '-') {
 > -                if (exp_digit || in_range)
 > -                    return -EINVAL;
 > -                b = 0;
 > -                in_range = 1;
 > -                exp_digit = 1;
 > -                continue;
 > -            }
 > -
 > -            if (!isdigit(c))
 > -                return -EINVAL;
 > -
 > -            b = b * 10 + (c - '0');
 > -            if (!in_range)
 > -                a = b;
 > -            exp_digit = 0;
 > -            totaldigits++;
 > +        endp = strchr(ibuf, ',');
 > +        if (endp)
 > +            *endp = '\0';
 > +        ibuf = strim(ibuf);
 > +        if (*ibuf == 0) {
 > +            ibuf = endp + 1;
 > +            continue;
 > +        }
 > +        ret = __bitmap_parse_common(ibuf, strlen(ibuf),
 > +                                &a, &b, 0);
 > +        if (ret)
 > +            break;
 > +        ibuf = endp + 1;
 > +
 > +        if (!(a <= b)) {
 > +            ret = -EINVAL;
 > +            break;
 > +        }
 > +        if (b >= nmaskbits) {
 > +            ret = -ERANGE;
 > +            break;
 >           }
 > -        if (!(a <= b))
 > -            return -EINVAL;
 > -        if (b >= nmaskbits)
 > -            return -ERANGE;
 >           while (a <= b) {
 >               set_bit(a, maskp);
 >               a++;
 >           }
 > -    } while (buflen && c == ',');
 > -    return 0;
 > +    } while (endp);
 > +    kfree(kbuf);
 > +    return ret;
 >   }
 >
 >   int bitmap_parselist(const char *bp, unsigned long *maskp, int 
nmaskbits)
 > --
 > 1.9.1