Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753715AbbGALLG (ORCPT ); Wed, 1 Jul 2015 07:11:06 -0400 Received: from mga01.intel.com ([192.55.52.88]:15577 "EHLO mga01.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753511AbbGALK6 (ORCPT ); Wed, 1 Jul 2015 07:10:58 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.15,385,1432623600"; d="scan'208";a="753954826" Message-ID: <5593CA29.1010703@intel.com> Date: Wed, 01 Jul 2015 19:08:25 +0800 From: Pan Xinhui User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: Yury , "linux-kernel@vger.kernel.org" CC: Andrew Morton , Rasmus Villemoes , tj@kernel.org, peterz@infradead.org, sudeep.holla@arm.com, mina86@mina86.com, "mnipxh@163.com" , Alexey Klimov Subject: Re: [PATCH V3] lib/bitmap.c: rewrite __bitmap_parse && __bitmap_parselist References: <5592487D.7080006@intel.com> <55934441.9010304@intel.com> <55924CD9.9020001@intel.com> <559253E0.2020602@intel.com> <55926929.4000901@intel.com> <5593AFFE.4060403@gmail.com> In-Reply-To: <5593AFFE.4060403@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 12507 Lines: 399 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 >> --- >> 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 >> #include >> >> +#include >> +#include >> /* >> * 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/