diff mbox

[v5,3/4] ndctl: convert region actions to use util_filter_walk()

Message ID 152486690886.66587.10650176680608823926.stgit@djiang5-desk3.ch.intel.com
State New, archived
Headers show

Commit Message

Jiang, Dave April 27, 2018, 10:08 p.m. UTC
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(-)

Comments

Verma, Vishal L May 9, 2018, 11:17 p.m. UTC | #1
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", &param.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, &param);
> +	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;
>  	};
>  };
>  
>
Dan Williams May 9, 2018, 11:23 p.m. UTC | #2
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", &param.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.
Jiang, Dave May 9, 2018, 11:24 p.m. UTC | #3
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", &param.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.
Dan Williams May 9, 2018, 11:26 p.m. UTC | #4
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", &param.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?
Verma, Vishal L May 9, 2018, 11:41 p.m. UTC | #5
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", &param.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..
Jiang, Dave May 11, 2018, 7:52 p.m. UTC | #6
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", &param.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?
Dan Williams May 11, 2018, 8:17 p.m. UTC | #7
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", &param.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 mbox

Patch

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", &param.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, &param);
+	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;
 	};
 };