On Fri, Jun 29, 2018 at 5:29 PM, Ross Zwisler
On Fri, Jun 29, 2018 at 04:57:29PM -0700, Dan Williams wrote:
> On Fri, Jun 29, 2018 at 4:39 PM, Ross Zwisler
> <ross.zwisler(a)linux.intel.com> wrote:
> > ndctl/list.c keeps track of a set of enum util_json_flags which are based
> > on the command line options passed in. This handling is a little messy,
> > though. We have an accessor function, listopts_to_flags(), which is used
> > multiple times. We sometimes pass the flags around as function arguments,
> > and we also have a copy stashed in the local struct list_filter_arg called
> > "lfa". In some functions we access the flags in multiple ways.
> > These flags are local to ndctl/list.c, are set exactly once per invocation
> > and never change. Create a variable local to that file, initialize it once
> > and use it everywhere.
> > Signed-off-by: Ross Zwisler <ross.zwisler(a)linux.intel.com>
> > ---
> > ndctl/list.c | 54 +++++++++++++++++++++++++++---------------------------
> > util/filter.h | 1 -
> > 2 files changed, 27 insertions(+), 28 deletions(-)
> I don't see the point of this thrash. It's not a win code size wise
> and I think it needlessly adds more dependence on global variables.
I disagree. We currently have the flags value passed around as function
arguments, we have a version stashed in a data structure we pass around, and
we have one which is already essentially a global variable that is accessed
via an accessor function. Having all three is complex and unnecessary,
especially considering that the flags never change.
Can we just choose one way of accessing the flags and use it everywhere? If
not a global variable, which would you like to use?
The point of having them passed into the util_filter_param was in
preparation for the "ndctl monitor" work. I'd prefer to see how that
work re-uses the list helpers or what can be refactored before we
convert more code to just look at a global variable. I agree that it
is currently pointless, but we are close to adding another use of
util_filter_param, so I'd prefer to see where that goes first.