| Message ID | 152486690886.66587.10650176680608823926.stgit@djiang5-desk3.ch.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 2018-04-27 at 15:08 -0700, Dave Jiang wrote: > util_filter_walk() does the looping through of busses and regions. > Removing > duplicate code in region ops and provide filter functions so we can > utilize util_filter_walk() and share common code. > > Signed-off-by: Dave Jiang <dave.jiang@intel.com> > --- > ndctl/region.c | 59 ++++++++++++++++++++++++++++++++++-------------- > -------- > util/filter.h | 6 ++++++ > 2 files changed, 42 insertions(+), 23 deletions(-) > > diff --git a/ndctl/region.c b/ndctl/region.c > index 9fc90808..9fd07af6 100644 > --- a/ndctl/region.c > +++ b/ndctl/region.c > @@ -19,10 +19,7 @@ > #include <util/parse-options.h> > #include <ndctl/libndctl.h> > > -static struct { > - const char *bus; > - const char *type; > -} param; > +struct util_filter_params param; > > static const struct option region_options[] = { > OPT_STRING('b', "bus", ¶m.bus, "bus-id", > @@ -92,33 +89,49 @@ static int region_action(struct ndctl_region *region, > enum device_action mode) > return 0; > } > > +static bool filter_bus(struct ndctl_bus *bus, struct util_filter_ctx > *ctx) > +{ > + return true; > +} > + Instead of creating these trivial functions everywhere (also applies to namespaces.c, dimm.c), should we just leave fctx.bus_filter NULL. And fix util_filter_walk to check for fctx->ptr != NULL any time it calls one of the functions.. > +static bool filter_region(struct ndctl_region *region, > + struct util_filter_ctx *ctx) > +{ > + struct rgaction_filter_arg *rfa = ctx->rgaction; > + int rc; > + > + if (rfa->rc < 0) > + return false; > + > + rc = region_action(region, rfa->action); > + > + if (rc == 0) > + rfa->rc++; > + else > + rfa->rc = 0; > + > + /* we don't need to fall through, can continue the loop */ > + return false; > +} > + > static int do_xable_region(const char *region_arg, enum device_action > mode, > struct ndctl_ctx *ctx) > { > - int rc = -ENXIO, success = 0; > - struct ndctl_region *region; > - struct ndctl_bus *bus; > + int rc = -ENXIO; > + struct util_filter_ctx fctx = { 0 }; > + struct rgaction_filter_arg rfa = { 0 }; > > if (!region_arg) > goto out; > > - ndctl_bus_foreach(ctx, bus) { > - if (!util_bus_filter(bus, param.bus)) > - continue; > - > - ndctl_region_foreach(bus, region) { > - const char *type = > ndctl_region_get_type_name(region); > - > - if (param.type && strcmp(param.type, type) != 0) > - continue; > - if (!util_region_filter(region, region_arg)) > - continue; > - if (region_action(region, mode) == 0) > - success++; > - } > - } > + fctx.filter_bus = filter_bus; > + fctx.filter_region = filter_region; > + fctx.rgaction = &rfa; > + fctx.rgaction->action = mode; > + rc = util_filter_walk(ctx, &fctx, ¶m); > + if (rc == 0) > + rc = fctx.rgaction->rc; > > - rc = success; > out: > param.bus = NULL; > return rc; > diff --git a/util/filter.h b/util/filter.h > index 9f3786a9..0e0e3bec 100644 > --- a/util/filter.h > +++ b/util/filter.h > @@ -57,6 +57,11 @@ struct nsaction_filter_arg { > int rc; > }; > > +struct rgaction_filter_arg { > + enum device_action action; > + int rc; > +}; > + > /* > * struct util_filter_ctx - control and callbacks for util_filter_walk() > * ->filter_bus() and ->filter_region() return bool because the > @@ -75,6 +80,7 @@ struct util_filter_ctx { > void *arg; > struct list_filter_arg *list; > struct nsaction_filter_arg *nsaction; > + struct rgaction_filter_arg *rgaction; > }; > }; > >
On Wed, May 9, 2018 at 4:17 PM, Verma, Vishal L <vishal.l.verma@intel.com> wrote: > On Fri, 2018-04-27 at 15:08 -0700, Dave Jiang wrote: >> util_filter_walk() does the looping through of busses and regions. >> Removing >> duplicate code in region ops and provide filter functions so we can >> utilize util_filter_walk() and share common code. >> >> Signed-off-by: Dave Jiang <dave.jiang@intel.com> >> --- >> ndctl/region.c | 59 ++++++++++++++++++++++++++++++++++-------------- >> -------- >> util/filter.h | 6 ++++++ >> 2 files changed, 42 insertions(+), 23 deletions(-) >> >> diff --git a/ndctl/region.c b/ndctl/region.c >> index 9fc90808..9fd07af6 100644 >> --- a/ndctl/region.c >> +++ b/ndctl/region.c >> @@ -19,10 +19,7 @@ >> #include <util/parse-options.h> >> #include <ndctl/libndctl.h> >> >> -static struct { >> - const char *bus; >> - const char *type; >> -} param; >> +struct util_filter_params param; >> >> static const struct option region_options[] = { >> OPT_STRING('b', "bus", ¶m.bus, "bus-id", >> @@ -92,33 +89,49 @@ static int region_action(struct ndctl_region *region, >> enum device_action mode) >> return 0; >> } >> >> +static bool filter_bus(struct ndctl_bus *bus, struct util_filter_ctx >> *ctx) >> +{ >> + return true; >> +} >> + > > Instead of creating these trivial functions everywhere (also applies to > namespaces.c, dimm.c), should we just leave fctx.bus_filter NULL. And fix > util_filter_walk to check for fctx->ptr != NULL any time it calls one of > the functions.. I think I'd prefer a common nop routine. That way individual are forced to decide to use the nop or implement something. Leaving it NULL may just be a programming mistake. I.e. it's harder to get wrong if it's required.
On 05/09/2018 04:23 PM, Dan Williams wrote: > On Wed, May 9, 2018 at 4:17 PM, Verma, Vishal L > <vishal.l.verma@intel.com> wrote: >> On Fri, 2018-04-27 at 15:08 -0700, Dave Jiang wrote: >>> util_filter_walk() does the looping through of busses and regions. >>> Removing >>> duplicate code in region ops and provide filter functions so we can >>> utilize util_filter_walk() and share common code. >>> >>> Signed-off-by: Dave Jiang <dave.jiang@intel.com> >>> --- >>> ndctl/region.c | 59 ++++++++++++++++++++++++++++++++++-------------- >>> -------- >>> util/filter.h | 6 ++++++ >>> 2 files changed, 42 insertions(+), 23 deletions(-) >>> >>> diff --git a/ndctl/region.c b/ndctl/region.c >>> index 9fc90808..9fd07af6 100644 >>> --- a/ndctl/region.c >>> +++ b/ndctl/region.c >>> @@ -19,10 +19,7 @@ >>> #include <util/parse-options.h> >>> #include <ndctl/libndctl.h> >>> >>> -static struct { >>> - const char *bus; >>> - const char *type; >>> -} param; >>> +struct util_filter_params param; >>> >>> static const struct option region_options[] = { >>> OPT_STRING('b', "bus", ¶m.bus, "bus-id", >>> @@ -92,33 +89,49 @@ static int region_action(struct ndctl_region *region, >>> enum device_action mode) >>> return 0; >>> } >>> >>> +static bool filter_bus(struct ndctl_bus *bus, struct util_filter_ctx >>> *ctx) >>> +{ >>> + return true; >>> +} >>> + >> >> Instead of creating these trivial functions everywhere (also applies to >> namespaces.c, dimm.c), should we just leave fctx.bus_filter NULL. And fix >> util_filter_walk to check for fctx->ptr != NULL any time it calls one of >> the functions.. > > I think I'd prefer a common nop routine. That way individual are > forced to decide to use the nop or implement something. Leaving it > NULL may just be a programming mistake. I.e. it's harder to get wrong > if it's required. > I'll add a common nop routine.
On Wed, May 9, 2018 at 4:24 PM, Dave Jiang <dave.jiang@intel.com> wrote: > > > On 05/09/2018 04:23 PM, Dan Williams wrote: >> On Wed, May 9, 2018 at 4:17 PM, Verma, Vishal L >> <vishal.l.verma@intel.com> wrote: >>> On Fri, 2018-04-27 at 15:08 -0700, Dave Jiang wrote: >>>> util_filter_walk() does the looping through of busses and regions. >>>> Removing >>>> duplicate code in region ops and provide filter functions so we can >>>> utilize util_filter_walk() and share common code. >>>> >>>> Signed-off-by: Dave Jiang <dave.jiang@intel.com> >>>> --- >>>> ndctl/region.c | 59 ++++++++++++++++++++++++++++++++++-------------- >>>> -------- >>>> util/filter.h | 6 ++++++ >>>> 2 files changed, 42 insertions(+), 23 deletions(-) >>>> >>>> diff --git a/ndctl/region.c b/ndctl/region.c >>>> index 9fc90808..9fd07af6 100644 >>>> --- a/ndctl/region.c >>>> +++ b/ndctl/region.c >>>> @@ -19,10 +19,7 @@ >>>> #include <util/parse-options.h> >>>> #include <ndctl/libndctl.h> >>>> >>>> -static struct { >>>> - const char *bus; >>>> - const char *type; >>>> -} param; >>>> +struct util_filter_params param; >>>> >>>> static const struct option region_options[] = { >>>> OPT_STRING('b', "bus", ¶m.bus, "bus-id", >>>> @@ -92,33 +89,49 @@ static int region_action(struct ndctl_region *region, >>>> enum device_action mode) >>>> return 0; >>>> } >>>> >>>> +static bool filter_bus(struct ndctl_bus *bus, struct util_filter_ctx >>>> *ctx) >>>> +{ >>>> + return true; >>>> +} >>>> + >>> >>> Instead of creating these trivial functions everywhere (also applies to >>> namespaces.c, dimm.c), should we just leave fctx.bus_filter NULL. And fix >>> util_filter_walk to check for fctx->ptr != NULL any time it calls one of >>> the functions.. >> >> I think I'd prefer a common nop routine. That way individual are >> forced to decide to use the nop or implement something. Leaving it >> NULL may just be a programming mistake. I.e. it's harder to get wrong >> if it's required. >> > > I'll add a common nop routine. ...but wait why would it be a nop in this case. Region action commands take a --bus= option?
On Wed, 2018-05-09 at 16:26 -0700, Dan Williams wrote: > On Wed, May 9, 2018 at 4:24 PM, Dave Jiang <dave.jiang@intel.com> wrote: > > > > > > On 05/09/2018 04:23 PM, Dan Williams wrote: > > > On Wed, May 9, 2018 at 4:17 PM, Verma, Vishal L > > > <vishal.l.verma@intel.com> wrote: > > > > On Fri, 2018-04-27 at 15:08 -0700, Dave Jiang wrote: > > > > > util_filter_walk() does the looping through of busses and > > > > > regions. > > > > > Removing > > > > > duplicate code in region ops and provide filter functions so we > > > > > can > > > > > utilize util_filter_walk() and share common code. > > > > > > > > > > Signed-off-by: Dave Jiang <dave.jiang@intel.com> > > > > > --- > > > > > ndctl/region.c | 59 ++++++++++++++++++++++++++++++++++------ > > > > > -------- > > > > > -------- > > > > > util/filter.h | 6 ++++++ > > > > > 2 files changed, 42 insertions(+), 23 deletions(-) > > > > > > > > > > diff --git a/ndctl/region.c b/ndctl/region.c > > > > > index 9fc90808..9fd07af6 100644 > > > > > --- a/ndctl/region.c > > > > > +++ b/ndctl/region.c > > > > > @@ -19,10 +19,7 @@ > > > > > #include <util/parse-options.h> > > > > > #include <ndctl/libndctl.h> > > > > > > > > > > -static struct { > > > > > - const char *bus; > > > > > - const char *type; > > > > > -} param; > > > > > +struct util_filter_params param; > > > > > > > > > > static const struct option region_options[] = { > > > > > OPT_STRING('b', "bus", ¶m.bus, "bus-id", > > > > > @@ -92,33 +89,49 @@ static int region_action(struct ndctl_region > > > > > *region, > > > > > enum device_action mode) > > > > > return 0; > > > > > } > > > > > > > > > > +static bool filter_bus(struct ndctl_bus *bus, struct > > > > > util_filter_ctx > > > > > *ctx) > > > > > +{ > > > > > + return true; > > > > > +} > > > > > + > > > > > > > > Instead of creating these trivial functions everywhere (also > > > > applies to > > > > namespaces.c, dimm.c), should we just leave fctx.bus_filter NULL. > > > > And fix > > > > util_filter_walk to check for fctx->ptr != NULL any time it calls > > > > one of > > > > the functions.. > > > > > > I think I'd prefer a common nop routine. That way individual are > > > forced to decide to use the nop or implement something. Leaving it > > > NULL may just be a programming mistake. I.e. it's harder to get wrong > > > if it's required. > > > > > > > I'll add a common nop routine. > > ...but wait why would it be a nop in this case. Region action commands > take a --bus= option? Ah yeah, and so do namespace and dimm actions right? Also, should we then error out in the util function in case a NULL is encountered? Should be better than just dereferencing it..
On 05/09/2018 04:26 PM, Dan Williams wrote: > On Wed, May 9, 2018 at 4:24 PM, Dave Jiang <dave.jiang@intel.com> wrote: >> >> >> On 05/09/2018 04:23 PM, Dan Williams wrote: >>> On Wed, May 9, 2018 at 4:17 PM, Verma, Vishal L >>> <vishal.l.verma@intel.com> wrote: >>>> On Fri, 2018-04-27 at 15:08 -0700, Dave Jiang wrote: >>>>> util_filter_walk() does the looping through of busses and regions. >>>>> Removing >>>>> duplicate code in region ops and provide filter functions so we can >>>>> utilize util_filter_walk() and share common code. >>>>> >>>>> Signed-off-by: Dave Jiang <dave.jiang@intel.com> >>>>> --- >>>>> ndctl/region.c | 59 ++++++++++++++++++++++++++++++++++-------------- >>>>> -------- >>>>> util/filter.h | 6 ++++++ >>>>> 2 files changed, 42 insertions(+), 23 deletions(-) >>>>> >>>>> diff --git a/ndctl/region.c b/ndctl/region.c >>>>> index 9fc90808..9fd07af6 100644 >>>>> --- a/ndctl/region.c >>>>> +++ b/ndctl/region.c >>>>> @@ -19,10 +19,7 @@ >>>>> #include <util/parse-options.h> >>>>> #include <ndctl/libndctl.h> >>>>> >>>>> -static struct { >>>>> - const char *bus; >>>>> - const char *type; >>>>> -} param; >>>>> +struct util_filter_params param; >>>>> >>>>> static const struct option region_options[] = { >>>>> OPT_STRING('b', "bus", ¶m.bus, "bus-id", >>>>> @@ -92,33 +89,49 @@ static int region_action(struct ndctl_region *region, >>>>> enum device_action mode) >>>>> return 0; >>>>> } >>>>> >>>>> +static bool filter_bus(struct ndctl_bus *bus, struct util_filter_ctx >>>>> *ctx) >>>>> +{ >>>>> + return true; >>>>> +} >>>>> + >>>> >>>> Instead of creating these trivial functions everywhere (also applies to >>>> namespaces.c, dimm.c), should we just leave fctx.bus_filter NULL. And fix >>>> util_filter_walk to check for fctx->ptr != NULL any time it calls one of >>>> the functions.. >>> >>> I think I'd prefer a common nop routine. That way individual are >>> forced to decide to use the nop or implement something. Leaving it >>> NULL may just be a programming mistake. I.e. it's harder to get wrong >>> if it's required. >>> >> >> I'll add a common nop routine. > > ...but wait why would it be a nop in this case. Region action commands > take a --bus= option? > It just passes through by return true because it doesn't do anything to do the bus? The util_bus_filter() call before it filters appropriate bus right?
On Fri, May 11, 2018 at 12:52 PM, Dave Jiang <dave.jiang@intel.com> wrote: > > > On 05/09/2018 04:26 PM, Dan Williams wrote: >> On Wed, May 9, 2018 at 4:24 PM, Dave Jiang <dave.jiang@intel.com> wrote: >>> >>> >>> On 05/09/2018 04:23 PM, Dan Williams wrote: >>>> On Wed, May 9, 2018 at 4:17 PM, Verma, Vishal L >>>> <vishal.l.verma@intel.com> wrote: >>>>> On Fri, 2018-04-27 at 15:08 -0700, Dave Jiang wrote: >>>>>> util_filter_walk() does the looping through of busses and regions. >>>>>> Removing >>>>>> duplicate code in region ops and provide filter functions so we can >>>>>> utilize util_filter_walk() and share common code. >>>>>> >>>>>> Signed-off-by: Dave Jiang <dave.jiang@intel.com> >>>>>> --- >>>>>> ndctl/region.c | 59 ++++++++++++++++++++++++++++++++++-------------- >>>>>> -------- >>>>>> util/filter.h | 6 ++++++ >>>>>> 2 files changed, 42 insertions(+), 23 deletions(-) >>>>>> >>>>>> diff --git a/ndctl/region.c b/ndctl/region.c >>>>>> index 9fc90808..9fd07af6 100644 >>>>>> --- a/ndctl/region.c >>>>>> +++ b/ndctl/region.c >>>>>> @@ -19,10 +19,7 @@ >>>>>> #include <util/parse-options.h> >>>>>> #include <ndctl/libndctl.h> >>>>>> >>>>>> -static struct { >>>>>> - const char *bus; >>>>>> - const char *type; >>>>>> -} param; >>>>>> +struct util_filter_params param; >>>>>> >>>>>> static const struct option region_options[] = { >>>>>> OPT_STRING('b', "bus", ¶m.bus, "bus-id", >>>>>> @@ -92,33 +89,49 @@ static int region_action(struct ndctl_region *region, >>>>>> enum device_action mode) >>>>>> return 0; >>>>>> } >>>>>> >>>>>> +static bool filter_bus(struct ndctl_bus *bus, struct util_filter_ctx >>>>>> *ctx) >>>>>> +{ >>>>>> + return true; >>>>>> +} >>>>>> + >>>>> >>>>> Instead of creating these trivial functions everywhere (also applies to >>>>> namespaces.c, dimm.c), should we just leave fctx.bus_filter NULL. And fix >>>>> util_filter_walk to check for fctx->ptr != NULL any time it calls one of >>>>> the functions.. >>>> >>>> I think I'd prefer a common nop routine. That way individual are >>>> forced to decide to use the nop or implement something. Leaving it >>>> NULL may just be a programming mistake. I.e. it's harder to get wrong >>>> if it's required. >>>> >>> >>> I'll add a common nop routine. >> >> ...but wait why would it be a nop in this case. Region action commands >> take a --bus= option? >> > > It just passes through by return true because it doesn't do anything to > do the bus? The util_bus_filter() call before it filters appropriate bus > right? Ah true, sorry, I confused myself on when the filter should return false.
diff --git a/ndctl/region.c b/ndctl/region.c index 9fc90808..9fd07af6 100644 --- a/ndctl/region.c +++ b/ndctl/region.c @@ -19,10 +19,7 @@ #include <util/parse-options.h> #include <ndctl/libndctl.h> -static struct { - const char *bus; - const char *type; -} param; +struct util_filter_params param; static const struct option region_options[] = { OPT_STRING('b', "bus", ¶m.bus, "bus-id", @@ -92,33 +89,49 @@ static int region_action(struct ndctl_region *region, enum device_action mode) return 0; } +static bool filter_bus(struct ndctl_bus *bus, struct util_filter_ctx *ctx) +{ + return true; +} + +static bool filter_region(struct ndctl_region *region, + struct util_filter_ctx *ctx) +{ + struct rgaction_filter_arg *rfa = ctx->rgaction; + int rc; + + if (rfa->rc < 0) + return false; + + rc = region_action(region, rfa->action); + + if (rc == 0) + rfa->rc++; + else + rfa->rc = 0; + + /* we don't need to fall through, can continue the loop */ + return false; +} + static int do_xable_region(const char *region_arg, enum device_action mode, struct ndctl_ctx *ctx) { - int rc = -ENXIO, success = 0; - struct ndctl_region *region; - struct ndctl_bus *bus; + int rc = -ENXIO; + struct util_filter_ctx fctx = { 0 }; + struct rgaction_filter_arg rfa = { 0 }; if (!region_arg) goto out; - ndctl_bus_foreach(ctx, bus) { - if (!util_bus_filter(bus, param.bus)) - continue; - - ndctl_region_foreach(bus, region) { - const char *type = ndctl_region_get_type_name(region); - - if (param.type && strcmp(param.type, type) != 0) - continue; - if (!util_region_filter(region, region_arg)) - continue; - if (region_action(region, mode) == 0) - success++; - } - } + fctx.filter_bus = filter_bus; + fctx.filter_region = filter_region; + fctx.rgaction = &rfa; + fctx.rgaction->action = mode; + rc = util_filter_walk(ctx, &fctx, ¶m); + if (rc == 0) + rc = fctx.rgaction->rc; - rc = success; out: param.bus = NULL; return rc; diff --git a/util/filter.h b/util/filter.h index 9f3786a9..0e0e3bec 100644 --- a/util/filter.h +++ b/util/filter.h @@ -57,6 +57,11 @@ struct nsaction_filter_arg { int rc; }; +struct rgaction_filter_arg { + enum device_action action; + int rc; +}; + /* * struct util_filter_ctx - control and callbacks for util_filter_walk() * ->filter_bus() and ->filter_region() return bool because the @@ -75,6 +80,7 @@ struct util_filter_ctx { void *arg; struct list_filter_arg *list; struct nsaction_filter_arg *nsaction; + struct rgaction_filter_arg *rgaction; }; };
util_filter_walk() does the looping through of busses and regions. Removing duplicate code in region ops and provide filter functions so we can utilize util_filter_walk() and share common code. Signed-off-by: Dave Jiang <dave.jiang@intel.com> --- ndctl/region.c | 59 ++++++++++++++++++++++++++++++++++---------------------- util/filter.h | 6 ++++++ 2 files changed, 42 insertions(+), 23 deletions(-)