linux-kernel - Re: [PATCH 5/8] lib: bitmap_getnum: separate arg into region and field

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]

Message-ID: <CAAH8bW9pQ_==uF3_Zo6K-ijqHXDVcW_-9gx0fvFWXe=fEvsg9A@mail.gmail.com>
Date:   Tue, 26 Jan 2021 18:58:46 -0800
From:   Yury Norov <yury.norov@...il.com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     Paul Gortmaker <paul.gortmaker@...driver.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

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.

> > 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))
> >
> >  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
>
>