Re: [PATCH] lib/bitmap.c: return -EINVAL for grouping errors in __bitmap_parselist

[Pan Xinhui]

hi, Yury

On 2015å06æ30æ 16:32, Yury Norov wrote:
> 2015-07-01 4:37 GMT+03:00 Pan Xinhui <xinhuix.pan@xxxxxxxxx>:
> > 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@xxxxxxxxx>
> > >
> > >
> > > 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/