2015-07-01 4:37 GMT+03:00 Pan Xinhui <xinhuix....@intel.com>: > hi, Yury > thanks for your nice reply. > > On 2015年06月29日 21:39, Yury Norov wrote: >>> >>> Sometimes the input from user may cause an unexpected result. >> >> >> Could you please provide specific example? >> > I wrote some scripts to do some tests about irqs. > echo "1-3," > /proc/irq/<xxx>/smp_affinity_list > this command ends with ',' by mistake. > actually __bitmap_parselist() will report "0-3" for the final result which > is wrong. >
Hmm... I don't think this is wrong passing echo "1-3,". With or without a comma, the final result must be the same. More flexible format is useful for hard scripts (for your one). It's not too difficult to imagine a script producing a line: "1-24, , ,,, , 12-64, 92,92,92,,," And I don't think we should reject user with this once the range is valid. Even more, to spend a time writing some additional code for it, and make user spend his time as well. I just tried cd /home/yury///./././/work and it works perfectly well for me, and it's fine. The true problem is that a and b variables goes zero after comma, and EOL after comma just takes it: 514 do { ... 517 a = b = 0; // <--- comma makes it 0 here ... 520 while (buflen) { ... 539 /* A '\0' or a ',' signal the end of a cpu# or range */ 540 if (c == '\0' || c == ',') // <---here we just break after '\0' 541 break; 559 } ... 565 while (a <= b) { 566 set_bit(a, maskp); // <--- and here we set unneeded 0 bit. 567 a++; 568 } So currently, "1-3,\0" is the same as "1-3,0,\0". And this is definitely wrong. > >>> >>> just like __bitmap_parse, we return -EINVAL if there is no avaiable digit >>> in each >>> parsing procedures. >>> >>> Signed-off-by: Pan Xinhui <xinhuix....@intel.com> >> >> >> Hello, Pan. >> >> (Adding Alexey Klimov, Rasmus Villemoes) >> >>> --- >>> lib/bitmap.c | 7 +++++-- >>> 1 file changed, 5 insertions(+), 2 deletions(-) >>> >>> diff --git a/lib/bitmap.c b/lib/bitmap.c >>> index 64c0926..995fca2 100644 >>> --- a/lib/bitmap.c >>> +++ b/lib/bitmap.c >>> @@ -504,7 +504,7 @@ static int __bitmap_parselist(const char *buf, >>> unsigned int buflen, >>> int nmaskbits) >>> { >>> unsigned a, b; >>> - int c, old_c, totaldigits; >>> + int c, old_c, totaldigits, ndigits; >>> const char __user __force *ubuf = (const char __user __force >>> *)buf; >>> int exp_digit, in_range; >>> >>> @@ -514,6 +514,7 @@ static int __bitmap_parselist(const char *buf, >>> unsigned int buflen, >>> exp_digit = 1; >>> in_range = 0; >>> a = b = 0; >>> + ndigits = 0; >>> >>> /* Get the next cpu# or a range of cpu#'s */ >>> while (buflen) { >>> @@ -555,8 +556,10 @@ static int __bitmap_parselist(const char *buf, >>> unsigned int buflen, >>> if (!in_range) >>> a = b; >>> exp_digit = 0; >>> - totaldigits++; >>> + ndigits++; totaldigits++; >> >> >> I'm not happy with joining two statements to a single line. >> Maybe sometimes it's OK for loop iterators like >> >> while (a[i][j]) { >> i++; j++; >> } >> >> But here it looks nasty. Anyway, it's minor. >> > > thanks for pointing out my mistake about the code style :) > >>> } >>> + if (ndigits == 0) >>> + return -EINVAL; >> >> >> You can avoid in-loop incrementation of ndigits if you'll >> save current totaldigits to ndigits before loop, and check >> ndigits against totaldigits after the loop: >> >> ndigits = totaldigits; >> while (...) { >> ... >> totaldigits++; >> } >> >> if (ndigits == totaldigits) >> return -EINVAL; >> >> Maybe it's a good point to rework initial __bitmap_parse() similar way... >> > > your advice is a good idea, thanks. > I am also thinking if we can rewrite them into one function for common > codes. > > thanks for your reply again :) > > thanks > xinhui > > >>> if (!(a <= b)) >>> return -EINVAL; >>> if (b >= nmaskbits) >>> -- >>> 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/