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

[Yury]

> 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@xxxxxxxxx>
> ---
>   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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/