Messages in this thread | | | Date | Tue, 30 Jun 2015 16:37:48 +0800 | From | Pan Xinhui <> | Subject | Re: [PATCH] lib/bitmap.c: return -EINVAL for grouping errors in __bitmap_parselist |
| |
hi, Yury
On 2015年06月30日 16:32, Yury Norov wrote: > 2015-07-01 4:37 GMT+03:00 Pan Xinhui <xinhuix.pan@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.pan@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
| |