lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Wed, 27 Jan 2021 03:38:10 -0500
From:   Paul Gortmaker <paul.gortmaker@...driver.com>
To:     Yury Norov <yury.norov@...il.com>
Cc:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        lizefan@...wei.com, Ingo Molnar <mingo@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>, josh@...htriplett.org,
        Peter Zijlstra <peterz@...radead.org>,
        "Paul E. McKenney" <paulmck@...nel.org>, fweisbec@...il.com,
        Rasmus Villemoes <linux@...musvillemoes.dk>
Subject: Re: [PATCH 5/8] lib: bitmap_getnum: separate arg into region and
 field

[Re: [PATCH 5/8] lib: bitmap_getnum: separate arg into region and field] On 26/01/2021 (Tue 18:58) Yury Norov wrote:

> On Tue, Jan 26, 2021 at 1:22 PM Andy Shevchenko
> <andriy.shevchenko@...ux.intel.com> wrote:
> >
> > On Tue, Jan 26, 2021 at 12:11:38PM -0500, Paul Gortmaker wrote:
> > > The bitmap_getnum is only used on a region's start/end/off/group_len
> > > field.  Trivially decouple the region from the field so that the region
> > > pointer is available for a pending change.
> >
> > Honestly, I don't like this macro trick. It's bad in couple of ways:
> >  - it hides what actually is done with the fields of r structure
> >    (after you get that they are fields!)
> >  - it breaks possibility to compile time (type) checks
> >
> > I will listen what others say, but I'm in favour not to proceed like this.
> 
> Agree. Would be better to drop the patch. Paul, what kind of pending
> change do you mean here? All the following patches are not related to
> parsing machinery.

It was directly related, because...

> 
> > > Cc: Yury Norov <yury.norov@...il.com>
> > > Cc: Rasmus Villemoes <linux@...musvillemoes.dk>
> > > Cc: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> > > Signed-off-by: Paul Gortmaker <paul.gortmaker@...driver.com>
> > > ---
> > >  lib/bitmap.c | 9 +++++----
> > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/lib/bitmap.c b/lib/bitmap.c
> > > index 833f152a2c43..f65be2f148fd 100644
> > > --- a/lib/bitmap.c
> > > +++ b/lib/bitmap.c
> > > @@ -533,6 +533,7 @@ static const char *bitmap_getnum(const char *str, unsigned int *num)
> > >       *num = n;
> > >       return str + len;
> > >  }
> > > +#define bitmap_getrnum(s, r, pos) bitmap_getnum(s, &(r->pos))

...this one line above opened the door to then do [in 6/8]:

   -#define bitmap_getrnum(s, r, pos) bitmap_getnum(s, &(r->pos))
   +#define bitmap_getrnum(s, r, pos) __bitmap_getnum(s, r->nbits, &(r->pos))

which gets nbits down into bitmap_getnum so we can handle N in there as
the placement you'd specifically requested for treating N as just a number.

In any case, I've decided against putting nbits into the region struct
and have got the nbits value down into getnum() another way for v4,
without using this commit or similar macros.

Paul.
--

> > >
> > >  static inline bool end_of_str(char c)
> > >  {
> > > @@ -571,7 +572,7 @@ static const char *bitmap_find_region_reverse(const char *start, const char *end
> > >
> > >  static const char *bitmap_parse_region(const char *str, struct region *r)
> > >  {
> > > -     str = bitmap_getnum(str, &r->start);
> > > +     str = bitmap_getrnum(str, r, start);
> > >       if (IS_ERR(str))
> > >               return str;
> > >
> > > @@ -581,7 +582,7 @@ static const char *bitmap_parse_region(const char *str, struct region *r)
> > >       if (*str != '-')
> > >               return ERR_PTR(-EINVAL);
> > >
> > > -     str = bitmap_getnum(str + 1, &r->end);
> > > +     str = bitmap_getrnum(str + 1, r, end);
> > >       if (IS_ERR(str))
> > >               return str;
> > >
> > > @@ -591,14 +592,14 @@ static const char *bitmap_parse_region(const char *str, struct region *r)
> > >       if (*str != ':')
> > >               return ERR_PTR(-EINVAL);
> > >
> > > -     str = bitmap_getnum(str + 1, &r->off);
> > > +     str = bitmap_getrnum(str + 1, r, off);
> > >       if (IS_ERR(str))
> > >               return str;
> > >
> > >       if (*str != '/')
> > >               return ERR_PTR(-EINVAL);
> > >
> > > -     return bitmap_getnum(str + 1, &r->group_len);
> > > +     return bitmap_getrnum(str + 1, r, group_len);
> > >
> > >  no_end:
> > >       r->end = r->start;
> > > --
> > > 2.17.1
> > >
> >
> > --
> > With Best Regards,
> > Andy Shevchenko
> >
> >

Powered by blists - more mailing lists