hi, Yury
On 2015年06月30日 16:32, Yury Norov wrote:
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.
yes, you are right.
current codes did not check if there is any digit between ',' or '\0'.
I has sent out patch V2, which rewrite two functions.
could you help have a code review if you have free time? thanks for your nice
reply :)
thanks,
xinhui
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/
