by Pan Xinhui

[permalink] [raw]

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

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 <[email protected]>
>> ---
>> 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
>