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

From: Dan Williams <dan.j.williams@intel.com>
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
Date: Mon, 9 Apr 2018 10:45:55 -0700	[thread overview]
Message-ID: <CAPcyv4iYWC3iY+u91PGOQ3aJoJBUbVM--KjtNCA834PTAE4E6Q@mail.gmail.com> (raw)
In-Reply-To: <0DEDF3B159719A448A49EF0E7B11E3222764CC51@g01jpexmbkw01>

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/libudev
>> > .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.

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