Re: [RFC PATCH v4] ndctl: monitor: add ndctl monitor daemon

["Verma, Vishal L" <vishal.l.verma@intel.com>]

On Mon, 2018-04-09 at 10:45 -0700, Dan Williams wrote:
> On Mon, Apr 9, 2018 at 1:38 AM, Qi, Fuli <qi.fuli@jp.fujitsu.com> wrote:
> > 
> > 
> > > -----Original Message-----
> > > From: Dan Williams [mailto:dan.j.williams@intel.com]
> > > Sent: Saturday, April 7, 2018 4:03 AM
> > > To: Qi, Fuli/斉 福利 <qi.fuli@jp.fujitsu.com>
> > > Cc: linux-nvdimm <linux-nvdimm@lists.01.org>
> > > Subject: Re: [RFC PATCH v4] ndctl: monitor: add ndctl monitor daemon
> > > 
> > > bus="<bus filter option>"
> > > 
> > > 
> > > On Thu, Apr 5, 2018 at 4:17 PM, Qi, Fuli <qi.fuli@jp.fujitsu.com>
> > > wrote:
> > > [..]
> > > > > This seems to needlessly tie ndctl to systemd, it should be able
> > > > > to
> > > > > operate without requiring systemd. I expect it would be
> > > > > straightforward to copy the configuration file implementation
> > > > > from git.
> > > > 
> > > > I have read the configuration file implementation of git, my
> > > > understanding is that git daemon does not have any options used to
> > > > override
> > > 
> > > default configuration.
> > > > I want to confirm if the configuration file is only used for ndctl
> > > > monitor.
> > > > If yes, I do not think copy the configuration file implementation
> > > > from
> > > > git is a good choice, because only getting keys and values from
> > > > configuration file is needed for us and the structure of
> > > > configuration file
> > > 
> > > implementation in git is too complexity.
> > > > I prefer to borrow from udev[1], because the implementation in udev
> > > > is simpler
> > > 
> > > and it seems ndctl also borrows a lot from udev.
> > > > 
> > > > [1]
> > > > https://git.kernel.org/pub/scm/linux/hotplug/udev.git/tree/src/libu
> > > > dev
> > > > .c
> > > 
> > > Thank you for doing the due diligence on this investigation it is
> > > appreciated.
> > > 
> > > I think the simple udev approach is acceptable. Going back to the
> > > proposed
> > > command line options of: --dimm-events, --namespace-events, --region-
> > > events,
> > > --bus-events and device filter selectors (like the ndctl list
> > > options) we can just have
> > > variables in the config file for those, so:
> > > 
> > > dimm-events="<list of dimm events>"
> > > namespace-events=="<list of namespace events>"
> > > region-events=="<list of region events>"
> > > bus-events="<list of bus events>"
> > > dimm="<bus filter option>"
> > > dimm="<dimm filter option>"
> > > region="<region filter option>"
> > > namespace="<namespace filter option>"
> > > 
> > > Thoughts?
> > 
> > Thank you very much.
> > 
> > I think the "logfile=<logfile path>" is also needed in the
> > configuration file.
> > 
> > One more question is that do you think it is necessary for ndctl list
> > to support "one option multiple arguments"?
> > Currently, only "one option one argument" is allowed in ndctl list like
> > "ndctl list --dimm nmem1 --bus ndbus1".
> > Due to monitor should support "one option multiple arguments" like
> > "ndctl monitor --dimm nmem1,nmem2",
> > I am thinking modify the "strut util_filter_params" in util/filter.h,
> > change the "const char *bus" to "char **buses"
> > or "link_list bus" and refactor the util_filter_walk in util/filter.h,
> > then ndctl list also can support "ndctl list --dimm nmem1,nmem2".
> 
> That would be a welcome feature. Given that we already support the
> "all" keyword to match multiple objects in a filter it should be
> straightforward to support lists of objects.
> 
> The only concern is syntax. I think since the label commands
> "init-labels, read-labels, write-labels" takes a space separated list
> of objects I think we should do the same specifying multiple object
> names to the filter.

Agreed with space separated arguments. While at it, it would also be nice
to extend the multiple argument syntax to other commands, for example: 

	ndctl disable-namespace namespace1.0 namespace2.0 ...

> 
> I.e. a request to list multiple specific dimms would be:
> 
>     ndctl list --dimm="nmem1 nmem2 nmem3"
> 
> ...space separation instead of comma allows you to rewrite that as:
> 
>     ndctl list --dimm="$(echo nmem{1,2,3)"
> _______________________________________________
> Linux-nvdimm mailing list
> Linux-nvdimm@lists.01.org
> https://lists.01.org/mailman/listinfo/linux-nvdimm
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm