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

[Yury Norov]

On Tue, Jan 26, 2021 at 1:22 PM Andy Shevchenko
<andriy.shevchenko@xxxxxxxxxxxxxxx> 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@xxxxxxxxx>
> > Cc: Rasmus Villemoes <linux@xxxxxxxxxxxxxxxxxx>
> > Cc: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> > Signed-off-by: Paul Gortmaker <paul.gortmaker@xxxxxxxxxxxxx>
> > ---
> >  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
>
>