lkml.org 
[lkml]   [2021]   [Feb]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 6/8] lib: bitmap: support "N" as an alias for size of bitmap
On Wed, Feb 10, 2021 at 10:58:25AM -0500, Paul Gortmaker wrote:
> [Re: [PATCH 6/8] lib: bitmap: support "N" as an alias for size of bitmap] On 09/02/2021 (Tue 15:16) Yury Norov wrote:
>
> > On Tue, Feb 9, 2021 at 3:01 PM Paul Gortmaker
> > <paul.gortmaker@windriver.com> wrote:
>
> [...]
>
> > > -static const char *bitmap_getnum(const char *str, unsigned int *num)
> > > +static const char *bitmap_getnum(const char *str, unsigned int *num,
> > > + unsigned int lastbit)
> >
> > The idea of struct bitmap_region is avoid passing the lastbit to the functions.
> > But here you do pass. Can you please be consistent? Or if I misunderstand
> > the idea of struct bitmap_region, can you please clarify it?
> >
> > Also, I don't think that in this specific case it's worth it to create
> > a hierarchy of
> > structures. Just adding lastbits to struct region will be simpler and more
> > transparent.
>
> I'm getting mixed messages from different people as to what is wanted here.
>
> Here is what the code looks like now; only relevant lines shown:
>
> -------------------------------
> int bitmap_parselist(const char *buf, unsigned long *maskp, int nmaskbits)
> {
>
> struct region r;
>
> bitmap_parse_region(buf, &r); <-----------
> bitmap_check_region(&r);
> bitmap_set_region(&r, maskp, nmaskbits);
> }
>
> static const char *bitmap_parse_region(const char *str, struct region *r)
> {
> bitmap_getnum(str, &r->start);
> bitmap_getnum(str + 1, &r->end);
> bitmap_getnum(str + 1, &r->off);
> bitmap_getnum(str + 1, &r->group_len);
> }
>
> static const char *bitmap_getnum(const char *str, unsigned int *num)
> {
> /* PG: We need nmaskbits here for N processing. */
> }
> -------------------------------
>
>
> Note the final function - the one where you asked to locate the N
> processing into -- does not take a region. So even if we bundle nbits
> into the region struct, it doesn't get the data to where we need it.
>
> Choices:
>
> 1) pass in nbits just like bitmap_set_region() does currently.
>
> 2) add nbits to region and pass full region instead of start/end/off.
>
> 2a) add nbits to region and pass full region and also start/end/off.
>
> 3) use *num as a bi-directional data path and initialize with nbits.
>
>
> Yury doesn't want us add any function args -- i.e. not to do #1.
>
> Andy didn't like #2 because it "hides" that we are writing to r.
>
> I ruled out sending 2a -- bitmap_getnum(str, r, &r->end) because
> it adds an arg, AND seems rather redundant to pass r and r->field.
>
> The #3 is the smallest change - but seems like we are trying to be
> too clever just to save a line of code or a couple bytes. (see below)
>
> Yury - in your reply to patch 5, you indicate you wrote the region
> code and want me to go back to putting nbits into region directly.
>
> Can you guys please clarify who is maintainer and hence exactly how
> you want this relatively minor detail handled? I'll gladly do it
> in whatever way the maintainer wants just to get this finally done.

Funny that there is no maintainer of the code.
That said, I consider #1 or #3 is good enough. Rationale for
- #1: it doesn't touch purity of getnum(), I think it's good enough not to know
region details
- #3 (as you posted below): I like how it looks like (one nit below, though)

But let's put this way, I think Yury had done a lot in the area, let's listen
more to him than to me.

> I'd rather not keep going in circles and guessing and annoying everyone
> else on the Cc: list by filling their inbox any more than I already have.
>
> That would help a lot in getting this finished.

Agree!

> Example #3 -- not sent..
>
> +#define DECLARE_REGION(rname, initval) \
> +struct region rname = { \
> + .start = initval, \
> + .off = initval, \
> + .group_len = initval, \
> + .end = initval, \
> +}
>
> [...]
>
> - struct region r;
> + DECLARE_REGION(r, nmaskbits - 1); /* "N-N:N/N" */

I would initialize with nmaskbits to be sure the value is invalid, but it will
add some code, below, so up to you, guys.

> +/*
> + * Seeing 'N' tells us to leave the value of "num" unchanged (which will
> + * be the max value for the width of the bitmap, set via DECLARE_REGION).
> + */
> static const char *bitmap_getnum(const char *str, unsigned int *num)
> {
> unsigned long long n;
> unsigned int len;
>
> + if (str[0] == 'N') /* nothing to do, just advance str */
> + return str + 1;
>

--
With Best Regards,
Andy Shevchenko


\
 
 \ /
  Last update: 2021-02-10 17:54    [W:0.135 / U:1.568 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site