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

[Pan Xinhui]

hi, Yury
	
On 2015å07æ01æ 17:16, Yury wrote:
>> 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

sorry for that I did not configure thunderbird correctly.
tab becomes spaces unexpectedly. :(

>>
>> 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.
> 
yes, this is a bad name.

> 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.
> 
I will try to find a better way. thanks for pointing out the bad codes design.

> 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);
>     }
> 

agree!
a minor change.
int parse_range(const char *buf, char *endp, unsigned long *start, unsigned long *len)
seems better, I prefer ret value as error codes only.

thanks for your advices. :)
I will rewrite them.

thanks,
xinhui

>> +{
>> +    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/