Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751909AbbF3Ike (ORCPT ); Tue, 30 Jun 2015 04:40:34 -0400 Received: from mga14.intel.com ([192.55.52.115]:16560 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751581AbbF3IkY (ORCPT ); Tue, 30 Jun 2015 04:40:24 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.15,376,1432623600"; d="scan'208";a="720161985" Message-ID: <5592555C.4080702@intel.com> Date: Tue, 30 Jun 2015 16:37:48 +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 Norov CC: "linux-kernel@vger.kernel.org" , 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] lib/bitmap.c: return -EINVAL for grouping errors in __bitmap_parselist References: <5592487D.7080006@intel.com> <55934441.9010304@intel.com> In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5138 Lines: 169 hi, Yury On 2015年06月30日 16:32, Yury Norov wrote: > 2015-07-01 4:37 GMT+03:00 Pan Xinhui : >> 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//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 >>> >>> >>> 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 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/