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.gortma...@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