[PATCH v6 0/4] ndctl, monitor: add ndctl monitor daemon

[PATCH v6 0/4] ndctl, monitor: add ndctl monitor daemon

[QI Fuli]

This is the v6 patch for ndctl monitor daemon, a tiny daemon to monitor
the smart events of nvdimm DIMMs. Users can run a monitor as a one-shot
command or a daemon in background by using the [--daemon] option. DIMMs
to monitor can be selected by [--dimm] [--bus] [--region] [--namespace]
options, these options support multiple space-seperated arguments.
When a smart event fires, monitor daemon will log the notifications
which including dimm health status to syslog or a logfile by setting
[--logfile=<file|syslog>] option. monitor also can output the
notifications to stderr when it run as one-shot command by setting
[--logfile=<stderr>]. The notifications follow json format and can be
consumed by log collectors like Fluentd. Users can change the
configuration of monitor by editing the default configuration file
/etc/ndctl/monitor.conf or by using [--config-file=<file>] option to
override the default configuration. 

Users can start a monitor daemon by the following command:
 # ndctl monitor --daemon --logfile /var/log/ndctl/monitor.log

Also, a monitor daemon can be started by systemd:
 # systemctl start ndctl-monitor.service
In this case, monitor daemon follows the default configuration file
/etc/ndctl/monitor.conf.

Signed-off-by: QI Fuli <qi.fuli@jp.fujitsu.com>

---
Change log since v5:
 - Fixing systemd unit file cannot be installed bug
 - Adding license to ./util/abspath.c

Change log since v4:
 - Adding OPTION_FILENAME to make sure filename is correct
 - Adding configuration file
 - Adding [--config-file] option to override the default configuration
 - Making some options support multiple space-seperated arguments
 - Making systemctl enable ndctl-monitor.service command work
 - Making systemctl restart ndctl-monitor.service command work
 - Making the directory of systemd unit file to be configurable
 - Changing log_file() and log_syslog() to logreport()
 - Changing date format in notification to nanoseconds since epoch
 - Changing select() to epoll()
 - Adding filter_bus() and filter_region()

Change log since v3:
 - Removing create-monitor, show-monitor, list-monitor, destroy-monitor
 - Adding [--daemon] option to run ndctl monitor as a daemon 
 - Using systemd to manage ndctl monitor daemon
 - Replacing filter_monitor_dimm() with filter_dimm()

Change log since v2:
 - Changing the interface of daemon to the ndctl command line
 - Changing the name of daemon form "nvdimmd" to "monitor"
 - Removing the config file, unit_file, nvdimmd dir
 - Removing nvdimmd_test program
 - Adding ndctl/monitor.c

Change log since v1:
 - Adding a config file(/etc/nvdimmd/nvdimmd.conf)
 - Using struct log_ctx instead of syslog()
    - Using log_syslog() to save the notify messages to syslog
    - Using log_file() to save the notify messages to special file
 - Adding LOG_NOTICE level to log_priority
 - Using automake instead of Makefile
 - Adding a new util file(nvdimmd/util.c) including helper functions
   needed for nvdimm daemon
 - Adding nvdimmd_test program

QI Fuli (4):
  ndctl, util: add OPTION_FILENAME to parse_opt_type
  ndctl, monitor: add ndctl monitor daemon
  ndctl, monitor: add default configuration file
  ndctl, monitor: add the unit file of systemd for ndctl-monitor service

 Makefile.am                 |   3 +-
 autogen.sh                  |   3 +-
 builtin.h                   |   1 +
 configure.ac                |  22 ++
 ndctl/Makefile.am           |  12 +-
 ndctl/monitor.c             | 460 ++++++++++++++++++++++++++++++++++++
 ndctl/monitor.conf          |  37 +++
 ndctl/ndctl-monitor.service |   7 +
 ndctl/ndctl.c               |   1 +
 util/abspath.c              |  29 +++
 util/help.c                 |   5 -
 util/parse-options.c        |  47 +++-
 util/parse-options.h        |  11 +-
 util/util.h                 |   7 +
 14 files changed, 629 insertions(+), 16 deletions(-)
 create mode 100644 ndctl/monitor.c
 create mode 100644 ndctl/monitor.conf
 create mode 100644 ndctl/ndctl-monitor.service
 create mode 100644 util/abspath.c

-- 
2.17.0.140.g0b0cc9f86


_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH v6 1/4] ndctl, util: add OPTION_FILENAME to parse_opt_type

[QI Fuli]

This patch borrows the OPTION_FILENAME from git to ndctl to make sure
filename is correct. Some related refactoring is also included:
  - adds parse_options_prefix() interface
  - moves is_absolute from util/help.c to util/util.c
  - adds a new file util/abspath.c

Signed-off-by: QI Fuli <qi.fuli@jp.fujitsu.com>
---
 Makefile.am          |  3 ++-
 util/abspath.c       | 29 +++++++++++++++++++++++++++
 util/help.c          |  5 -----
 util/parse-options.c | 47 ++++++++++++++++++++++++++++++++++++++------
 util/parse-options.h | 11 +++++++++--
 util/util.h          |  7 +++++++
 6 files changed, 88 insertions(+), 14 deletions(-)
 create mode 100644 util/abspath.c

diff --git a/Makefile.am b/Makefile.am
index b538b1f..e0c463a 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -69,6 +69,7 @@ libutil_a_SOURCES = \
 	util/strbuf.c \
 	util/wrapper.c \
 	util/filter.c \
-	util/bitmap.c
+	util/bitmap.c \
+	util/abspath.c
 
 nobase_include_HEADERS = daxctl/libdaxctl.h
diff --git a/util/abspath.c b/util/abspath.c
new file mode 100644
index 0000000..09bbd27
--- /dev/null
+++ b/util/abspath.c
@@ -0,0 +1,29 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* originally copied from git/abspath.c */
+
+#include <util/util.h>
+#include <util/strbuf.h>
+
+char *prefix_filename(const char *pfx, const char *arg)
+{
+	struct strbuf path = STRBUF_INIT;
+	size_t pfx_len = pfx ? strlen(pfx) : 0;
+
+	if (!pfx_len)
+		;
+	else if (is_absolute_path(arg))
+		pfx_len = 0;
+	else
+		strbuf_add(&path, pfx, pfx_len);
+
+	strbuf_addstr(&path, arg);
+	return strbuf_detach(&path, NULL);
+}
+
+void fix_filename(const char *prefix, const char **file)
+{
+	if (!file || !*file || !prefix || is_absolute_path(*file)
+			|| !strcmp("-", *file))
+		return;
+	*file = prefix_filename(prefix, *file);
+}
diff --git a/util/help.c b/util/help.c
index 8b8f951..2d57fa1 100644
--- a/util/help.c
+++ b/util/help.c
@@ -89,11 +89,6 @@ static char *cmd_to_page(const char *cmd, char **page, const char *util_name)
 	return *page;
 }
 
-static int is_absolute_path(const char *path)
-{
-	return path[0] == '/';
-}
-
 static const char *system_path(const char *path)
 {
         static const char *prefix = PREFIX;
diff --git a/util/parse-options.c b/util/parse-options.c
index 751c091..c781174 100644
--- a/util/parse-options.c
+++ b/util/parse-options.c
@@ -55,6 +55,7 @@ static int get_value(struct parse_opt_ctx_t *p,
 {
 	const char *s, *arg = NULL;
 	const int unset = flags & OPT_UNSET;
+	int err;
 
 	if (unset && p->opt)
 		return opterror(opt, "takes no value", flags);
@@ -77,6 +78,7 @@ static int get_value(struct parse_opt_ctx_t *p,
 		case OPTION_ARGUMENT:
 		case OPTION_GROUP:
 		case OPTION_STRING:
+		case OPTION_FILENAME:
 		case OPTION_INTEGER:
 		case OPTION_UINTEGER:
 		case OPTION_LONG:
@@ -121,6 +123,19 @@ static int get_value(struct parse_opt_ctx_t *p,
 			return get_arg(p, opt, flags, (const char **)opt->value);
 		return 0;
 
+	case OPTION_FILENAME:
+		err = 0;
+		if (unset)
+			*(const char **)opt->value = NULL;
+		else if (opt->flags & PARSE_OPT_OPTARG && !p->opt)
+			*(const char **)opt->value = (const char *)opt->defval;
+		else
+			err = get_arg(p, opt, flags, (const char **)opt->value);
+
+		if (!err)
+			fix_filename(p->prefix, (const char **)opt->value);
+		return err;
+
 	case OPTION_CALLBACK:
 		if (unset)
 			return (*opt->callback)(opt, NULL, 1) ? (-1) : 0;
@@ -339,13 +354,14 @@ static void check_typos(const char *arg, const struct option *options)
 	}
 }
 
-void parse_options_start(struct parse_opt_ctx_t *ctx,
-			 int argc, const char **argv, int flags)
+void parse_options_start(struct parse_opt_ctx_t *ctx, int argc,
+			const char **argv, const char *prefix, int flags)
 {
 	memset(ctx, 0, sizeof(*ctx));
 	ctx->argc = argc - 1;
 	ctx->argv = argv + 1;
 	ctx->out  = argv;
+	ctx->prefix = prefix;
 	ctx->cpidx = ((flags & PARSE_OPT_KEEP_ARGV0) != 0);
 	ctx->flags = flags;
 	if ((flags & PARSE_OPT_KEEP_UNKNOWN) &&
@@ -453,8 +469,10 @@ int parse_options_end(struct parse_opt_ctx_t *ctx)
 	return ctx->cpidx + ctx->argc;
 }
 
-int parse_options_subcommand(int argc, const char **argv, const struct option *options,
-			const char *const subcommands[], const char *usagestr[], int flags)
+static int parse_options_subcommand_prefix(int argc, const char **argv,
+			const char *prefix, const struct option *options,
+			const char *const subcommands[],
+			const char *usagestr[], int flags)
 {
 	struct parse_opt_ctx_t ctx;
 
@@ -474,7 +492,7 @@ int parse_options_subcommand(int argc, const char **argv, const struct option *o
 		strbuf_release(&buf);
 	}
 
-	parse_options_start(&ctx, argc, argv, flags);
+	parse_options_start(&ctx, argc, argv, prefix, flags);
 	switch (parse_options_step(&ctx, options, usagestr)) {
 	case PARSE_OPT_HELP:
 		exit(129);
@@ -503,10 +521,26 @@ int parse_options_subcommand(int argc, const char **argv, const struct option *o
 	return parse_options_end(&ctx);
 }
 
+int parse_options_subcommand(int argc, const char **argv,
+		const struct option *options, const char *const subcommands[],
+		const char *usagestr[], int flags)
+{
+	return parse_options_subcommand_prefix(argc, argv, NULL, options,
+					subcommands, usagestr, flags);
+}
+
+int parse_options_prefix(int argc, const char **argv, const char *prefix,
+			const struct option *options,
+			const char * const usagestr[], int flags)
+{
+	return parse_options_subcommand_prefix(argc, argv, prefix, options,
+				NULL, (const char **) usagestr, flags);
+}
+
 int parse_options(int argc, const char **argv, const struct option *options,
 		  const char * const usagestr[], int flags)
 {
-	return parse_options_subcommand(argc, argv, options, NULL,
+	return parse_options_subcommand_prefix(argc, argv, NULL, options, NULL,
 					(const char **) usagestr, flags);
 }
 
@@ -557,6 +591,7 @@ static void print_option_help(const struct option *opts, int full)
 		if (opts->flags & PARSE_OPT_NOARG)
 			break;
 		/* FALLTHROUGH */
+	case OPTION_FILENAME:
 	case OPTION_STRING:
 		if (opts->argh) {
 			if (opts->flags & PARSE_OPT_OPTARG)
diff --git a/util/parse-options.h b/util/parse-options.h
index 6fd6b24..fc5015a 100644
--- a/util/parse-options.h
+++ b/util/parse-options.h
@@ -38,6 +38,7 @@ enum parse_opt_type {
 	OPTION_CALLBACK,
 	OPTION_U64,
 	OPTION_UINTEGER,
+	OPTION_FILENAME,
 };
 
 enum parse_opt_flags {
@@ -135,6 +136,7 @@ struct option {
 #define OPT_LONG(s, l, v, h)        { .type = OPTION_LONG, .short_name = (s), .long_name = (l), .value = check_vtype(v, long *), .help = (h) }
 #define OPT_U64(s, l, v, h)         { .type = OPTION_U64, .short_name = (s), .long_name = (l), .value = check_vtype(v, u64 *), .help = (h) }
 #define OPT_STRING(s, l, v, a, h)   { .type = OPTION_STRING,  .short_name = (s), .long_name = (l), .value = check_vtype(v, const char **), (a), .help = (h) }
+#define OPT_FILENAME(s, l, v, a, h) { .type = OPTION_FILENAME, .short_name = (s), .long_name = (l), .value = check_vtype(v, const char **), (a), .help = (h) }
 #define OPT_DATE(s, l, v, h) \
 	{ .type = OPTION_CALLBACK, .short_name = (s), .long_name = (l), .value = (v), .argh = "time", .help = (h), .callback = parse_opt_approxidate_cb }
 #define OPT_CALLBACK(s, l, v, a, h, f) \
@@ -156,6 +158,10 @@ extern int parse_options(int argc, const char **argv,
                          const struct option *options,
                          const char * const usagestr[], int flags);
 
+extern int parse_options_prefix(int argc, const char **argv,
+			const char *prefix, const struct option *options,
+			const char * const usagestr[], int flags);
+
 extern int parse_options_subcommand(int argc, const char **argv,
 				const struct option *options,
 				const char *const subcommands[],
@@ -185,6 +191,7 @@ struct parse_opt_ctx_t {
 	int argc, cpidx;
 	const char *opt;
 	int flags;
+	const char *prefix;
 };
 
 extern int parse_options_usage(const char * const *usagestr,
@@ -192,8 +199,8 @@ extern int parse_options_usage(const char * const *usagestr,
 			       const char *optstr,
 			       bool short_opt);
 
-extern void parse_options_start(struct parse_opt_ctx_t *ctx,
-				int argc, const char **argv, int flags);
+extern void parse_options_start(struct parse_opt_ctx_t *ctx, int argc,
+			const char **argv, const char *prefix, int flags);
 
 extern int parse_options_step(struct parse_opt_ctx_t *ctx,
 			      const struct option *options,
diff --git a/util/util.h b/util/util.h
index 162aade..001707e 100644
--- a/util/util.h
+++ b/util/util.h
@@ -79,6 +79,11 @@ static inline const char *skip_prefix(const char *str, const char *prefix)
         return strncmp(str, prefix, len) ? NULL : str + len;
 }
 
+static inline int is_absolute_path(const char *path)
+{
+	return path[0] == '/';
+}
+
 void usage(const char *err) NORETURN;
 void die(const char *err, ...) NORETURN __attribute__((format (printf, 1, 2)));
 int error(const char *err, ...) __attribute__((format (printf, 1, 2)));
@@ -87,5 +92,7 @@ void set_die_routine(void (*routine)(const char *err, va_list params) NORETURN);
 char *xstrdup(const char *str);
 void *xrealloc(void *ptr, size_t size);
 int prefixcmp(const char *str, const char *prefix);
+char *prefix_filename(const char *pfx, const char *arg);
+void fix_filename(const char *prefix, const char **file);
 
 #endif /* __UTIL_H__ */
-- 
2.17.0.140.g0b0cc9f86


_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v6 1/4] ndctl, util: add OPTION_FILENAME to parse_opt_type

[Dan Williams]

On Sun, May 6, 2018 at 10:09 PM, QI Fuli <qi.fuli@jp.fujitsu.com> wrote:
> This patch borrows the OPTION_FILENAME from git to ndctl to make sure
> filename is correct. Some related refactoring is also included:
>   - adds parse_options_prefix() interface
>   - moves is_absolute from util/help.c to util/util.c
>   - adds a new file util/abspath.c
>
> Signed-off-by: QI Fuli <qi.fuli@jp.fujitsu.com>

Reviewed-by: Dan Williams <dan.j.williams@intel.com>
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

RE: [PATCH v6 1/4] ndctl, util: add OPTION_FILENAME to parse_opt_type

[Qi, Fuli]

> -----Original Message-----
> From: Dan Williams [mailto:dan.j.williams@intel.com]
> Sent: Saturday, May 12, 2018 1:42 AM
> To: Qi, Fuli/斉 福利 <qi.fuli@jp.fujitsu.com>
> Cc: linux-nvdimm <linux-nvdimm@lists.01.org>
> Subject: Re: [PATCH v6 1/4] ndctl, util: add OPTION_FILENAME to parse_opt_type
> 
> On Sun, May 6, 2018 at 10:09 PM, QI Fuli <qi.fuli@jp.fujitsu.com> wrote:
> > This patch borrows the OPTION_FILENAME from git to ndctl to make sure
> > filename is correct. Some related refactoring is also included:
> >   - adds parse_options_prefix() interface
> >   - moves is_absolute from util/help.c to util/util.c
> >   - adds a new file util/abspath.c
> >
> > Signed-off-by: QI Fuli <qi.fuli@jp.fujitsu.com>
> 
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> 
Since this patch is independent from the others, could you please merge this one first?
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v6 1/4] ndctl, util: add OPTION_FILENAME to parse_opt_type

[Verma, Vishal L]

On Wed, 2018-05-16 at 02:42 +0000, Qi, Fuli wrote:
> > -----Original Message-----
> > From: Dan Williams [mailto:dan.j.williams@intel.com]
> > Sent: Saturday, May 12, 2018 1:42 AM
> > To: Qi, Fuli/斉 福利 <qi.fuli@jp.fujitsu.com>
> > Cc: linux-nvdimm <linux-nvdimm@lists.01.org>
> > Subject: Re: [PATCH v6 1/4] ndctl, util: add OPTION_FILENAME to
> > parse_opt_type
> > 
> > On Sun, May 6, 2018 at 10:09 PM, QI Fuli <qi.fuli@jp.fujitsu.com>
> > wrote:
> > > This patch borrows the OPTION_FILENAME from git to ndctl to make sure
> > > filename is correct. Some related refactoring is also included:
> > >   - adds parse_options_prefix() interface
> > >   - moves is_absolute from util/help.c to util/util.c
> > >   - adds a new file util/abspath.c
> > > 
> > > Signed-off-by: QI Fuli <qi.fuli@jp.fujitsu.com>
> > 
> > Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> > 
> 
> Since this patch is independent from the others, could you please merge
> this one first?

Hi Qi,

Yes I have applied the first patch, and it is now in the pending branch.

Thanks,
	-Vishal
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

RE: [PATCH v6 1/4] ndctl, util: add OPTION_FILENAME to parse_opt_type

[Qi, Fuli]

> -----Original Message-----
> From: Verma, Vishal L [mailto:vishal.l.verma@intel.com]
> Sent: Wednesday, June 13, 2018 7:45 AM
> To: Williams, Dan J <dan.j.williams@intel.com>; Qi, Fuli/斉 福利
> <qi.fuli@jp.fujitsu.com>
> Cc: linux-nvdimm@lists.01.org
> Subject: Re: [PATCH v6 1/4] ndctl, util: add OPTION_FILENAME to
> parse_opt_type
> 
> On Wed, 2018-05-16 at 02:42 +0000, Qi, Fuli wrote:
> > > -----Original Message-----
> > > From: Dan Williams [mailto:dan.j.williams@intel.com]
> > > Sent: Saturday, May 12, 2018 1:42 AM
> > > To: Qi, Fuli/斉 福利 <qi.fuli@jp.fujitsu.com>
> > > Cc: linux-nvdimm <linux-nvdimm@lists.01.org>
> > > Subject: Re: [PATCH v6 1/4] ndctl, util: add OPTION_FILENAME to
> > > parse_opt_type
> > >
> > > On Sun, May 6, 2018 at 10:09 PM, QI Fuli <qi.fuli@jp.fujitsu.com>
> > > wrote:
> > > > This patch borrows the OPTION_FILENAME from git to ndctl to make
> > > > sure filename is correct. Some related refactoring is also included:
> > > >   - adds parse_options_prefix() interface
> > > >   - moves is_absolute from util/help.c to util/util.c
> > > >   - adds a new file util/abspath.c
> > > >
> > > > Signed-off-by: QI Fuli <qi.fuli@jp.fujitsu.com>
> > >
> > > Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> > >
> >
> > Since this patch is independent from the others, could you please
> > merge this one first?
> 
> Hi Qi,
> 
> Yes I have applied the first patch, and it is now in the pending branch.
> 
> Thanks,
> 	-Vishal

Thank you very much.

Qi
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH v6 2/4] ndctl, monitor: add ndctl monitor daemon

[QI Fuli]

This patch adds the body file of ndctl monitor daemon.

Signed-off-by: QI Fuli <qi.fuli@jp.fujitsu.com>
---
 builtin.h         |   1 +
 ndctl/Makefile.am |   3 +-
 ndctl/monitor.c   | 460 ++++++++++++++++++++++++++++++++++++++++++++++
 ndctl/ndctl.c     |   1 +
 4 files changed, 464 insertions(+), 1 deletion(-)
 create mode 100644 ndctl/monitor.c

diff --git a/builtin.h b/builtin.h
index d3cc723..675a6ce 100644
--- a/builtin.h
+++ b/builtin.h
@@ -39,6 +39,7 @@ int cmd_inject_error(int argc, const char **argv, void *ctx);
 int cmd_wait_scrub(int argc, const char **argv, void *ctx);
 int cmd_start_scrub(int argc, const char **argv, void *ctx);
 int cmd_list(int argc, const char **argv, void *ctx);
+int cmd_monitor(int argc, const char **argv, void *ctx);
 #ifdef ENABLE_TEST
 int cmd_test(int argc, const char **argv, void *ctx);
 #endif
diff --git a/ndctl/Makefile.am b/ndctl/Makefile.am
index d22a379..7dbf223 100644
--- a/ndctl/Makefile.am
+++ b/ndctl/Makefile.am
@@ -16,7 +16,8 @@ ndctl_SOURCES = ndctl.c \
 		util/json-smart.c \
 		util/json-firmware.c \
 		inject-error.c \
-		inject-smart.c
+		inject-smart.c \
+		monitor.c
 
 if ENABLE_DESTRUCTIVE
 ndctl_SOURCES += ../test/blk_namespaces.c \
diff --git a/ndctl/monitor.c b/ndctl/monitor.c
new file mode 100644
index 0000000..ab6e701
--- /dev/null
+++ b/ndctl/monitor.c
@@ -0,0 +1,460 @@
+/*
+ * Copyright(c) 2018, FUJITSU LIMITED. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU Lesser General Public License,
+ * version 2.1, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT ANY
+ * WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
+ * FOR A PARTICULAR PURPOSE.  See the GNU Lesser General Public License for
+ * more details.
+ */
+
+#include <stdio.h>
+#include <json-c/json.h>
+#include <libgen.h>
+#include <dirent.h>
+#include <util/log.h>
+#include <util/json.h>
+#include <util/filter.h>
+#include <util/util.h>
+#include <util/parse-options.h>
+#include <util/strbuf.h>
+#include <ndctl/lib/private.h>
+#include <ndctl/libndctl.h>
+#include <sys/stat.h>
+#include <sys/epoll.h>
+#define BUF_SIZE 2048
+
+static enum log_destination {
+	LOG_DESTINATION_STDERR = 1,
+	LOG_DESTINATION_SYSLOG = 2,
+	LOG_DESTINATION_FILE = 3,
+} log_destination = LOG_DESTINATION_SYSLOG;
+
+static struct {
+	const char *logfile;
+	const char *config_file;
+	bool daemon;
+} monitor;
+
+struct monitor_dimm_node {
+	struct ndctl_dimm *dimm;
+	int health_eventfd;
+	struct list_node list;
+};
+
+struct monitor_filter_arg {
+	struct list_head mdimm;
+	int maxfd_dimm;
+	int num_dimm;
+	unsigned long flags;
+};
+
+struct util_filter_params param;
+
+static int did_fail;
+
+#define fail(fmt, ...) \
+do { \
+	did_fail = 1; \
+	err(ctx, "ndctl-%s:%s:%d: " fmt, \
+			VERSION, __func__, __LINE__, ##__VA_ARGS__); \
+} while (0)
+
+static bool is_dir(char *filepath)
+{
+	DIR *dir = opendir(filepath);
+	if (dir) {
+		closedir(dir);
+		return true;
+	}
+	return false;
+}
+
+static int recur_mkdir(char *filepath, mode_t mode)
+{
+	char *p;
+	char *buf = (char *)malloc(strlen(filepath) + 4);
+
+	strcpy(buf, filepath);
+	for (p = strchr(buf + 1, '/'); p; p = strchr(p + 1, '/')) {
+		*p = '\0';
+		if (!is_dir(buf)) {
+			if (mkdir(buf, mode) < 0) {
+				free(buf);
+				return -1;
+			}
+		}
+		*p = '/';
+	}
+
+	free(buf);
+	return 0;
+}
+
+static void set_value(const char **arg, char *val)
+{
+	struct strbuf value = STRBUF_INIT;
+	size_t arg_len = *arg ? strlen(*arg) : 0;
+
+	if (arg_len) {
+		strbuf_add(&value, *arg, arg_len);
+		strbuf_addstr(&value, " ");
+	}
+	strbuf_addstr(&value, val);
+	*arg = strbuf_detach(&value, NULL);
+}
+
+static void logreport(struct ndctl_ctx *ctx, int priority, const char *file,
+		int line, const char *fn, const char *format, va_list args)
+{
+	char *log_dir;
+	char *buf = (char *)malloc(BUF_SIZE);
+	vsnprintf(buf, BUF_SIZE, format, args);
+
+	switch (log_destination) {
+	case LOG_DESTINATION_STDERR:
+		fprintf(stderr, "%s\n", buf);
+		goto end;
+
+	case LOG_DESTINATION_SYSLOG:
+		syslog(priority, "%s\n", buf);
+		goto end;
+
+	case LOG_DESTINATION_FILE:
+		log_dir = dirname(strdup(monitor.logfile));
+		if (!is_dir(log_dir) && recur_mkdir(log_dir, 0744) != 0)
+			goto log_file_err;
+		FILE *f = fopen(monitor.logfile, "a+");
+		if (!f)
+			goto log_file_err;
+		fprintf(f, "%s\n", buf);
+		fclose(f);
+		free(log_dir);
+		goto end;
+
+	log_file_err:
+		log_destination = LOG_DESTINATION_SYSLOG;
+		fail("open logfile %s failed\n%s", monitor.logfile, buf);
+		free(log_dir);
+	}
+end:
+	free(buf);
+	return;
+}
+
+static void notify_json_msg(struct ndctl_ctx *ctx, struct ndctl_dimm *dimm)
+{
+	struct json_object *jmsg, *jdatetime, *jpid, *jdimm, *jhealth;
+	struct timespec ts;
+	char datetime[32];
+
+	jmsg = json_object_new_object();
+	if (!jmsg) {
+		fail("\n");
+		return;
+	}
+
+	clock_gettime(CLOCK_REALTIME, &ts);
+	sprintf(datetime, "%10ld.%09ld", ts.tv_sec, ts.tv_nsec);
+	jdatetime = json_object_new_string(datetime);
+	if (!jdatetime) {
+		fail("\n");
+		return;
+	}
+	json_object_object_add(jmsg, "datetime", jdatetime);
+
+	jpid = json_object_new_int((int)getpid());
+	if (!jpid) {
+		fail("\n");
+		return;
+	}
+	json_object_object_add(jmsg, "pid", jpid);
+
+	jdimm = util_dimm_to_json(dimm, 0);
+	if (!dimm) {
+		fail("\n");
+		return;
+	}
+	json_object_object_add(jmsg, "dimm", jdimm);
+
+	jhealth = util_dimm_health_to_json(dimm);
+	if (!jhealth) {
+		fail("\n");
+		return;
+	}
+	json_object_object_add(jdimm, "health", jhealth);
+
+	notice(ctx, "%s",
+		json_object_to_json_string_ext(jmsg, JSON_C_TO_STRING_PLAIN));
+}
+
+static bool filter_region(struct ndctl_region *region,
+		struct util_filter_ctx *ctx)
+{
+	return true;
+}
+
+static void filter_dimm(struct ndctl_dimm *dimm, struct util_filter_ctx *ctx)
+{
+	struct monitor_filter_arg *mfa = (struct monitor_filter_arg *)ctx->arg;
+
+	if (!ndctl_dimm_is_cmd_supported(dimm, ND_CMD_SMART_THRESHOLD))
+		return;
+
+	struct monitor_dimm_node *mdn = malloc(sizeof(*mdn));
+	mdn->dimm = dimm;
+	int fd = ndctl_dimm_get_health_eventfd(dimm);
+	mdn->health_eventfd = fd;
+	list_add_tail(&mfa->mdimm, &mdn->list);
+	if (fd > mfa->maxfd_dimm)
+		mfa->maxfd_dimm = fd;
+	mfa->num_dimm++;
+}
+
+static bool filter_bus(struct ndctl_bus *bus, struct util_filter_ctx *ctx)
+{
+	return true;
+}
+
+static int monitor_dimm_event(struct ndctl_ctx *ctx,
+		struct monitor_filter_arg *mfa)
+{
+	struct epoll_event ev, events[mfa->num_dimm];
+	struct ndctl_dimm **dimms;
+	int nfds, epollfd;
+	struct monitor_dimm_node *mdn;
+	char buf;
+
+	dimms = calloc(mfa->maxfd_dimm + 1, sizeof(struct ndctl_dimm *));
+	if (!dimms) {
+		fail("\n");
+		goto out;
+	}
+
+	epollfd = epoll_create1(0);
+	if (epollfd == -1) {
+		err(ctx, "epoll_create1 error\n");
+		goto out;
+	}
+	list_for_each(&mfa->mdimm, mdn, list) {
+		memset(&ev, 0, sizeof(ev));
+		pread(mdn->health_eventfd, &buf, 1, 0);
+		ev.data.fd = mdn->health_eventfd;
+		if (epoll_ctl(epollfd, EPOLL_CTL_ADD,
+				mdn->health_eventfd, &ev) != 0) {
+			err(ctx, "epoll_ctl error\n");
+			goto out;
+		}
+		dimms[mdn->health_eventfd] = mdn->dimm;
+	}
+
+	while(1){
+		nfds = epoll_wait(epollfd, events, mfa->num_dimm, -1);
+		if (nfds <= 0) {
+			err(ctx, "epoll_wait error\n");
+			goto out;
+		}
+		for (int i = 0; i < nfds; i++) {
+			memset(&ev, 0, sizeof(ev));
+			ev = events[i];
+			notify_json_msg(ctx, dimms[ev.data.fd]);
+			pread(ev.data.fd, &buf, 1, 0);
+		}
+		if (did_fail)
+			goto out;
+	}
+	return 0;
+out:
+	free(dimms);
+	return 1;
+}
+
+static int read_config_file(struct ndctl_ctx *ctx, const char *prefix)
+{
+	FILE *f;
+	int line_nr = 0;
+	char buf[BUF_SIZE];
+	char *config_file = "/etc/ndctl/monitor.conf";
+
+	if (monitor.config_file)
+		f = fopen(monitor.config_file, "r");
+	else
+		f = fopen(config_file, "r");
+
+	if (f == NULL) {
+		error("config-file: %s cannot be opened\n", config_file);
+		return -1;
+	}
+
+	while (fgets(buf, BUF_SIZE, f)) {
+		size_t len;
+		char *key;
+		char *val;
+
+		line_nr++;
+
+		/* find key */
+		key = buf;
+		while (isspace(key[0]))
+			key++;
+
+		/* comment or empty line */
+		if (key[0] == '#' || key[0] == '\0')
+			continue;
+
+		/* split key/value */
+		val = strchr(key, '=');
+		if (!val) {
+			err(ctx, "missing <key>=<value> in '%s'[%i], skip line\n",
+					config_file, line_nr);
+			continue;
+		}
+
+		val[0] = '\0';
+		val++;
+
+		/* find value */
+		while (isspace(val[0]))
+			val++;
+
+		/* terminate key */
+		len = strlen(key);
+		if (len == 0)
+			continue;
+		while (isspace(key[len-1]))
+			len--;
+		key[len] = '\0';
+
+		/*terminate value */
+		len = strlen(val);
+		if (len == 0)
+			continue;
+		while (isspace(val[len-1]))
+			len--;
+		val[len] = '\0';
+
+		if (len == 0)
+			continue;
+
+		/* unquote */
+		if (val[0] == '"' || val[0] == '\'') {
+			if (val[len-1] != val[0]) {
+				err(ctx, "inconsistent quoting in '%s'[%i], skip line\n",
+						config_file, line_nr);
+				continue;
+			}
+			val[len-1] = '\0';
+			val++;
+		}
+
+		if (strcmp(key, "bus") == 0) {
+			set_value((const char **)&param.bus, val);
+			continue;
+		}
+		if (strcmp(key, "dimm") == 0) {
+			set_value((const char **)&param.dimm, val);
+			continue;
+		}
+		if (strcmp(key, "region") == 0) {
+			set_value((const char **)&param.region, val);
+			continue;
+		}
+		if (strcmp(key, "namespace") == 0) {
+			set_value((const char **)&param.namespace, val);
+			continue;
+		}
+		if (strcmp(key, "logfile") == 0) {
+			if (monitor.logfile)
+				continue;
+			set_value((const char **)&monitor.logfile, val);
+			fix_filename(prefix, (const char **)&monitor.logfile);
+		}
+	}
+	fclose(f);
+	return 0;
+}
+
+int cmd_monitor(int argc, const char **argv, void *ctx)
+{
+	const struct option options[] = {
+		OPT_STRING('b', "bus", &param.bus, "bus-id", "filter by bus"),
+		OPT_STRING('r', "region", &param.region, "region-id",
+				"filter by region"),
+		OPT_STRING('d', "dimm", &param.dimm, "dimm-id",
+				"filter by dimm"),
+		OPT_STRING('n', "namespace", &param.namespace,
+				"namespace-id", "filter by namespace id"),
+		OPT_FILENAME('l', "logfile", &monitor.logfile, "file|syslog|stderr",
+				"the place to output the monitor's notification"),
+		OPT_FILENAME('c',"config-file", &monitor.config_file, "config-file",
+				"use file to override the default configuration"),
+		OPT_BOOLEAN('D',"daemon", &monitor.daemon,
+				"run ndctl monitor as a daemon"),
+		OPT_END(),
+	};
+	const char * const u[] = {
+		"ndctl monitor [<options>]",
+		NULL
+	};
+	const char *prefix = "./";
+	struct util_filter_ctx fctx = { 0 };
+	struct monitor_filter_arg mfa = { 0 };
+
+	argc = parse_options_prefix(argc, argv, prefix, options, u, 0);
+	for (int i = 0; i < argc; i++) {
+		error("unknown parameter \"%s\"\n", argv[i]);
+	}
+	if (argc)
+		usage_with_options(u, options);
+
+	ndctl_set_log_fn((struct ndctl_ctx *)ctx, logreport);
+	ndctl_set_log_priority((struct ndctl_ctx *)ctx, LOG_INFO);
+
+	if (read_config_file((struct ndctl_ctx *)ctx, prefix))
+		goto out;
+
+	if (monitor.logfile) {
+		if (strcmp(monitor.logfile, "./stderr") == 0)
+			log_destination = LOG_DESTINATION_STDERR;
+		else if (strcmp(monitor.logfile, "./syslog") == 0)
+			log_destination = LOG_DESTINATION_SYSLOG;
+		else
+			log_destination = LOG_DESTINATION_FILE;
+	}
+
+	if (monitor.daemon) {
+		if (daemon(0, 0) != 0) {
+			err((struct ndctl_ctx *)ctx, "daemon start failed\n");
+			goto out;
+		}
+		info((struct ndctl_ctx *)ctx, "ndctl monitor daemon started\n");
+	}
+
+	fctx.filter_bus = filter_bus;
+	fctx.filter_dimm = filter_dimm;
+	fctx.filter_region = filter_region;
+	fctx.filter_namespace = NULL;
+	fctx.arg = &mfa;
+	list_head_init(&mfa.mdimm);
+	mfa.num_dimm = 0;
+	mfa.maxfd_dimm = -1;
+	mfa.flags = 0;
+
+	if (util_filter_walk(ctx, &fctx, &param))
+		goto out;
+
+	if (!mfa.num_dimm) {
+		err((struct ndctl_ctx *)ctx, "no dimms can be monitored\n");
+		goto out;
+	}
+
+	if (monitor_dimm_event(ctx, &mfa))
+		goto out;
+
+	return 0;
+out:
+	return 1;
+}
diff --git a/ndctl/ndctl.c b/ndctl/ndctl.c
index 7daadeb..73dabfa 100644
--- a/ndctl/ndctl.c
+++ b/ndctl/ndctl.c
@@ -89,6 +89,7 @@ static struct cmd_struct commands[] = {
 	{ "wait-scrub", cmd_wait_scrub },
 	{ "start-scrub", cmd_start_scrub },
 	{ "list", cmd_list },
+	{ "monitor", cmd_monitor},
 	{ "help", cmd_help },
 	#ifdef ENABLE_TEST
 	{ "test", cmd_test },
-- 
2.17.0.140.g0b0cc9f86


_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v6 2/4] ndctl, monitor: add ndctl monitor daemon

[Dan Williams]

On Sun, May 6, 2018 at 10:09 PM, QI Fuli <qi.fuli@jp.fujitsu.com> wrote:
> This patch adds the body file of ndctl monitor daemon.

This is too short. Let's copy your cover letter details into this
patch since the cover letter is thrown away, but the commit messages
are preserved in git:

---

ndctl monitor daemon, a tiny daemon to monitor the smart events of
nvdimm DIMMs. Users can run a monitor as a one-shot command or a
daemon in background by using the [--daemon] option. DIMMs to monitor
can be selected by [--dimm] [--bus] [--region] [--namespace] options,
these options support multiple space-seperated arguments. When a smart
event fires, monitor daemon will log the notifications which including
dimm health status to syslog or a logfile by setting
[--logfile=<file|syslog>] option. monitor also can output the
notifications to stderr when it run as one-shot command by setting
[--logfile=<stderr>]. The notifications follow json format and can be
consumed by log collectors like Fluentd. Users can change the
configuration of monitor by editing the default configuration file
/etc/ndctl/monitor.conf or by using [--config-file=<file>] option to
override the default configuration.

Users can start a monitor daemon by the following command:
 # ndctl monitor --daemon --logfile /var/log/ndctl/monitor.log

Also, a monitor daemon can be started by systemd:
 # systemctl start ndctl-monitor.service
In this case, monitor daemon follows the default configuration file
/etc/ndctl/monitor.conf.

---

However, now that I re-read this description what about the other
event types (beyond health) on other objects (beyond DIMMs). This
should behave like the 'list' command where we have filter parameters
for devices to monitor *and* event types for events to include:

dimm-events="<list of dimm events>"
namespace-events="<list of namespace events>"
region-events="<list of region events>"
bus-events="<list of bus events>"
bus="<bus filter option>"
dimm="<dimm filter option>"
region="<region filter option>"
namespace="<namespace filter option>"

We don't need to support all of this in this first implementation, but
see more comments below I think there are some changes we can make to
start down this path.

>
> Signed-off-by: QI Fuli <qi.fuli@jp.fujitsu.com>
> ---
>  builtin.h         |   1 +
>  ndctl/Makefile.am |   3 +-
>  ndctl/monitor.c   | 460 ++++++++++++++++++++++++++++++++++++++++++++++
>  ndctl/ndctl.c     |   1 +
>  4 files changed, 464 insertions(+), 1 deletion(-)
>  create mode 100644 ndctl/monitor.c
>
> diff --git a/builtin.h b/builtin.h
> index d3cc723..675a6ce 100644
> --- a/builtin.h
> +++ b/builtin.h
> @@ -39,6 +39,7 @@ int cmd_inject_error(int argc, const char **argv, void *ctx);
>  int cmd_wait_scrub(int argc, const char **argv, void *ctx);
>  int cmd_start_scrub(int argc, const char **argv, void *ctx);
>  int cmd_list(int argc, const char **argv, void *ctx);
> +int cmd_monitor(int argc, const char **argv, void *ctx);
>  #ifdef ENABLE_TEST
>  int cmd_test(int argc, const char **argv, void *ctx);
>  #endif
> diff --git a/ndctl/Makefile.am b/ndctl/Makefile.am
> index d22a379..7dbf223 100644
> --- a/ndctl/Makefile.am
> +++ b/ndctl/Makefile.am
> @@ -16,7 +16,8 @@ ndctl_SOURCES = ndctl.c \
>                 util/json-smart.c \
>                 util/json-firmware.c \
>                 inject-error.c \
> -               inject-smart.c
> +               inject-smart.c \
> +               monitor.c
>
>  if ENABLE_DESTRUCTIVE
>  ndctl_SOURCES += ../test/blk_namespaces.c \
> diff --git a/ndctl/monitor.c b/ndctl/monitor.c
> new file mode 100644
> index 0000000..ab6e701
> --- /dev/null
> +++ b/ndctl/monitor.c
> @@ -0,0 +1,460 @@
> +/*
> + * Copyright(c) 2018, FUJITSU LIMITED. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU Lesser General Public License,
> + * version 2.1, as published by the Free Software Foundation.

Did you intend for this to be LGPL-2.1?

The licensing we have been doing to date is GPL-2.0 for the utility
code (i.e. ndctl/) especially because it copies from git which is
GPL2.0. LGPL-2.1 is for the library routines (i.e. ndctl/lib/). The
intent is for applications to be able to use the library without
needing to share their application source, but improvements to the
utility are shared back with the project.

> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT ANY
> + * WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
> + * FOR A PARTICULAR PURPOSE.  See the GNU Lesser General Public License for
> + * more details.
> + */

Convert this to SPDX style:

/* SPDX-License-Identifier: GPL-2.0 */
/* Copyright(c) 2018, FUJITSU LIMITED. All rights reserved. */

> +
> +#include <stdio.h>
> +#include <json-c/json.h>
> +#include <libgen.h>
> +#include <dirent.h>
> +#include <util/log.h>
> +#include <util/json.h>
> +#include <util/filter.h>
> +#include <util/util.h>
> +#include <util/parse-options.h>
> +#include <util/strbuf.h>
> +#include <ndctl/lib/private.h>
> +#include <ndctl/libndctl.h>
> +#include <sys/stat.h>
> +#include <sys/epoll.h>
> +#define BUF_SIZE 2048
> +
> +static enum log_destination {
> +       LOG_DESTINATION_STDERR = 1,
> +       LOG_DESTINATION_SYSLOG = 2,
> +       LOG_DESTINATION_FILE = 3,
> +} log_destination = LOG_DESTINATION_SYSLOG;
> +
> +static struct {
> +       const char *logfile;
> +       const char *config_file;
> +       bool daemon;
> +} monitor;
> +
> +struct monitor_dimm_node {
> +       struct ndctl_dimm *dimm;
> +       int health_eventfd;
> +       struct list_node list;
> +};
> +
> +struct monitor_filter_arg {
> +       struct list_head mdimm;

Let's call this field 'dimms' to match the list_head naming style in
other parts of the code.

> +       int maxfd_dimm;
> +       int num_dimm;
> +       unsigned long flags;
> +};
> +
> +struct util_filter_params param;
> +
> +static int did_fail;
> +
> +#define fail(fmt, ...) \
> +do { \
> +       did_fail = 1; \
> +       err(ctx, "ndctl-%s:%s:%d: " fmt, \
> +                       VERSION, __func__, __LINE__, ##__VA_ARGS__); \
> +} while (0)
> +
> +static bool is_dir(char *filepath)
> +{
> +       DIR *dir = opendir(filepath);
> +       if (dir) {
> +               closedir(dir);
> +               return true;
> +       }
> +       return false;
> +}
> +
> +static int recur_mkdir(char *filepath, mode_t mode)
> +{
> +       char *p;
> +       char *buf = (char *)malloc(strlen(filepath) + 4);
> +
> +       strcpy(buf, filepath);
> +       for (p = strchr(buf + 1, '/'); p; p = strchr(p + 1, '/')) {
> +               *p = '\0';
> +               if (!is_dir(buf)) {
> +                       if (mkdir(buf, mode) < 0) {
> +                               free(buf);
> +                               return -1;
> +                       }
> +               }
> +               *p = '/';
> +       }
> +
> +       free(buf);
> +       return 0;
> +}
> +
> +static void set_value(const char **arg, char *val)
> +{
> +       struct strbuf value = STRBUF_INIT;
> +       size_t arg_len = *arg ? strlen(*arg) : 0;
> +
> +       if (arg_len) {
> +               strbuf_add(&value, *arg, arg_len);
> +               strbuf_addstr(&value, " ");
> +       }
> +       strbuf_addstr(&value, val);
> +       *arg = strbuf_detach(&value, NULL);
> +}
> +
> +static void logreport(struct ndctl_ctx *ctx, int priority, const char *file,
> +               int line, const char *fn, const char *format, va_list args)
> +{
> +       char *log_dir;
> +       char *buf = (char *)malloc(BUF_SIZE);
> +       vsnprintf(buf, BUF_SIZE, format, args);
> +
> +       switch (log_destination) {
> +       case LOG_DESTINATION_STDERR:
> +               fprintf(stderr, "%s\n", buf);
> +               goto end;
> +
> +       case LOG_DESTINATION_SYSLOG:
> +               syslog(priority, "%s\n", buf);
> +               goto end;
> +
> +       case LOG_DESTINATION_FILE:
> +               log_dir = dirname(strdup(monitor.logfile));

We leak a memory allocation here, or crash if strdup fails.

> +               if (!is_dir(log_dir) && recur_mkdir(log_dir, 0744) != 0)
> +                       goto log_file_err;

No need to create the parent directory structure. Just fail if the
parent directories are not present. Require the administrator to
create the directories ahead of time.

Actually, I don't see a need to have LOG_DESTINATION_FILE at all. Why
not just do:

    ndctl monitor 2>file

...to redirect stderr to a file?


> +               FILE *f = fopen(monitor.logfile, "a+");
> +               if (!f)
> +                       goto log_file_err;
> +               fprintf(f, "%s\n", buf);
> +               fclose(f);
> +               free(log_dir);
> +               goto end;
> +
> +       log_file_err:
> +               log_destination = LOG_DESTINATION_SYSLOG;
> +               fail("open logfile %s failed\n%s", monitor.logfile, buf);
> +               free(log_dir);
> +       }
> +end:
> +       free(buf);
> +       return;
> +}
> +
> +static void notify_json_msg(struct ndctl_ctx *ctx, struct ndctl_dimm *dimm)
> +{
> +       struct json_object *jmsg, *jdatetime, *jpid, *jdimm, *jhealth;
> +       struct timespec ts;
> +       char datetime[32];
> +
> +       jmsg = json_object_new_object();
> +       if (!jmsg) {
> +               fail("\n");
> +               return;
> +       }
> +
> +       clock_gettime(CLOCK_REALTIME, &ts);
> +       sprintf(datetime, "%10ld.%09ld", ts.tv_sec, ts.tv_nsec);
> +       jdatetime = json_object_new_string(datetime);
> +       if (!jdatetime) {
> +               fail("\n");
> +               return;
> +       }
> +       json_object_object_add(jmsg, "datetime", jdatetime);

Let's call this field 'timestamp'.

> +
> +       jpid = json_object_new_int((int)getpid());

No need to cast here.

> +       if (!jpid) {
> +               fail("\n");
> +               return;
> +       }
> +       json_object_object_add(jmsg, "pid", jpid);
> +
> +       jdimm = util_dimm_to_json(dimm, 0);
> +       if (!dimm) {
> +               fail("\n");
> +               return;
> +       }
> +       json_object_object_add(jmsg, "dimm", jdimm);
> +
> +       jhealth = util_dimm_health_to_json(dimm);
> +       if (!jhealth) {
> +               fail("\n");
> +               return;
> +       }
> +       json_object_object_add(jdimm, "health", jhealth);
> +
> +       notice(ctx, "%s",
> +               json_object_to_json_string_ext(jmsg, JSON_C_TO_STRING_PLAIN));

As mentioned above this function seems to assume that the only DIMM
events to send are DIMM health events. It's ok to save other object
monitoring to a later patch, but let's at least support DIMM health
events:

dimm-spares-remaining
dimm-media-temperature
dimm-controller-temperature
dimm-health-state

...and DIMM detected events:

dimm-unclean-shutdown
dimm-detected

There should be an event type included in the json. Along with
'timestamp' and 'pid' I think we need an 'event' field so that
consumer code can make assumptions about the format of the event
record. I think 'dimm-health' and 'dimm-detect' are the only event
record types we need to support in this initial version.

> +}
> +
> +static bool filter_region(struct ndctl_region *region,
> +               struct util_filter_ctx *ctx)
> +{
> +       return true;
> +}
> +
> +static void filter_dimm(struct ndctl_dimm *dimm, struct util_filter_ctx *ctx)
> +{
> +       struct monitor_filter_arg *mfa = (struct monitor_filter_arg *)ctx->arg;
> +
> +       if (!ndctl_dimm_is_cmd_supported(dimm, ND_CMD_SMART_THRESHOLD))
> +               return;

Similar to above this assumes only health events, let's make the code
more generic.

> +
> +       struct monitor_dimm_node *mdn = malloc(sizeof(*mdn));
> +       mdn->dimm = dimm;
> +       int fd = ndctl_dimm_get_health_eventfd(dimm);

Let's not declare new variables in the middle of the function. I'll go
add "-Wdeclaration-after-statement" to the default warning flags.

> +       mdn->health_eventfd = fd;
> +       list_add_tail(&mfa->mdimm, &mdn->list);
> +       if (fd > mfa->maxfd_dimm)
> +               mfa->maxfd_dimm = fd;
> +       mfa->num_dimm++;
> +}
> +
> +static bool filter_bus(struct ndctl_bus *bus, struct util_filter_ctx *ctx)
> +{
> +       return true;
> +}
> +
> +static int monitor_dimm_event(struct ndctl_ctx *ctx,
> +               struct monitor_filter_arg *mfa)
> +{
> +       struct epoll_event ev, events[mfa->num_dimm];

Allocate events with malloc rather than a VLA.

> +       struct ndctl_dimm **dimms;
> +       int nfds, epollfd;
> +       struct monitor_dimm_node *mdn;
> +       char buf;
> +
> +       dimms = calloc(mfa->maxfd_dimm + 1, sizeof(struct ndctl_dimm *));

Why a separate allocation? I believe you can store pointers back to
the 'dimm' objects in the epoll_event data.

> +       if (!dimms) {
> +               fail("\n");
> +               goto out;
> +       }
> +
> +       epollfd = epoll_create1(0);
> +       if (epollfd == -1) {
> +               err(ctx, "epoll_create1 error\n");
> +               goto out;
> +       }
> +       list_for_each(&mfa->mdimm, mdn, list) {
> +               memset(&ev, 0, sizeof(ev));

Why memset?

> +               pread(mdn->health_eventfd, &buf, 1, 0);
> +               ev.data.fd = mdn->health_eventfd;
> +               if (epoll_ctl(epollfd, EPOLL_CTL_ADD,
> +                               mdn->health_eventfd, &ev) != 0) {
> +                       err(ctx, "epoll_ctl error\n");
> +                       goto out;
> +               }
> +               dimms[mdn->health_eventfd] = mdn->dimm;
> +       }
> +
> +       while(1){

Follow kernel coding style, i.e. add spaces: "while (1) {"

> +               nfds = epoll_wait(epollfd, events, mfa->num_dimm, -1);
> +               if (nfds <= 0) {
> +                       err(ctx, "epoll_wait error\n");
> +                       goto out;
> +               }
> +               for (int i = 0; i < nfds; i++) {

Move declaration of 'i' to the top of the function.

> +                       memset(&ev, 0, sizeof(ev));

Why memset?

> +                       ev = events[i];
> +                       notify_json_msg(ctx, dimms[ev.data.fd]);
> +                       pread(ev.data.fd, &buf, 1, 0);
> +               }
> +               if (did_fail)
> +                       goto out;
> +       }
> +       return 0;
> +out:
> +       free(dimms);
> +       return 1;
> +}
> +
> +static int read_config_file(struct ndctl_ctx *ctx, const char *prefix)

This should move to its own source file in util/.

> +{
> +       FILE *f;
> +       int line_nr = 0;
> +       char buf[BUF_SIZE];
> +       char *config_file = "/etc/ndctl/monitor.conf";
> +
> +       if (monitor.config_file)
> +               f = fopen(monitor.config_file, "r");
> +       else
> +               f = fopen(config_file, "r");
> +
> +       if (f == NULL) {
> +               error("config-file: %s cannot be opened\n", config_file);
> +               return -1;
> +       }
> +
> +       while (fgets(buf, BUF_SIZE, f)) {
> +               size_t len;
> +               char *key;
> +               char *val;
> +
> +               line_nr++;
> +
> +               /* find key */
> +               key = buf;
> +               while (isspace(key[0]))
> +                       key++;
> +
> +               /* comment or empty line */
> +               if (key[0] == '#' || key[0] == '\0')
> +                       continue;
> +
> +               /* split key/value */
> +               val = strchr(key, '=');
> +               if (!val) {
> +                       err(ctx, "missing <key>=<value> in '%s'[%i], skip line\n",
> +                                       config_file, line_nr);
> +                       continue;
> +               }
> +
> +               val[0] = '\0';
> +               val++;
> +
> +               /* find value */
> +               while (isspace(val[0]))
> +                       val++;
> +
> +               /* terminate key */
> +               len = strlen(key);
> +               if (len == 0)
> +                       continue;
> +               while (isspace(key[len-1]))
> +                       len--;
> +               key[len] = '\0';
> +
> +               /*terminate value */
> +               len = strlen(val);
> +               if (len == 0)
> +                       continue;
> +               while (isspace(val[len-1]))
> +                       len--;
> +               val[len] = '\0';
> +
> +               if (len == 0)
> +                       continue;
> +
> +               /* unquote */
> +               if (val[0] == '"' || val[0] == '\'') {
> +                       if (val[len-1] != val[0]) {
> +                               err(ctx, "inconsistent quoting in '%s'[%i], skip line\n",
> +                                               config_file, line_nr);
> +                               continue;
> +                       }
> +                       val[len-1] = '\0';
> +                       val++;
> +               }
> +
> +               if (strcmp(key, "bus") == 0) {
> +                       set_value((const char **)&param.bus, val);
> +                       continue;
> +               }
> +               if (strcmp(key, "dimm") == 0) {
> +                       set_value((const char **)&param.dimm, val);
> +                       continue;
> +               }
> +               if (strcmp(key, "region") == 0) {
> +                       set_value((const char **)&param.region, val);
> +                       continue;
> +               }
> +               if (strcmp(key, "namespace") == 0) {
> +                       set_value((const char **)&param.namespace, val);
> +                       continue;
> +               }
> +               if (strcmp(key, "logfile") == 0) {
> +                       if (monitor.logfile)
> +                               continue;
> +                       set_value((const char **)&monitor.logfile, val);
> +                       fix_filename(prefix, (const char **)&monitor.logfile);
> +               }
> +       }
> +       fclose(f);
> +       return 0;

Given that you said you borrowed this configuration file format from
udev, can we borrow their parsing code as well. I'd rather not carry
custom parsing code in ndctl.

> +}
> +
> +int cmd_monitor(int argc, const char **argv, void *ctx)
> +{
> +       const struct option options[] = {
> +               OPT_STRING('b', "bus", &param.bus, "bus-id", "filter by bus"),
> +               OPT_STRING('r', "region", &param.region, "region-id",
> +                               "filter by region"),
> +               OPT_STRING('d', "dimm", &param.dimm, "dimm-id",
> +                               "filter by dimm"),
> +               OPT_STRING('n', "namespace", &param.namespace,
> +                               "namespace-id", "filter by namespace id"),
> +               OPT_FILENAME('l', "logfile", &monitor.logfile, "file|syslog|stderr",
> +                               "the place to output the monitor's notification"),
> +               OPT_FILENAME('c',"config-file", &monitor.config_file, "config-file",
> +                               "use file to override the default configuration"),
> +               OPT_BOOLEAN('D',"daemon", &monitor.daemon,
> +                               "run ndctl monitor as a daemon"),
> +               OPT_END(),
> +       };
> +       const char * const u[] = {
> +               "ndctl monitor [<options>]",
> +               NULL
> +       };
> +       const char *prefix = "./";
> +       struct util_filter_ctx fctx = { 0 };
> +       struct monitor_filter_arg mfa = { 0 };
> +
> +       argc = parse_options_prefix(argc, argv, prefix, options, u, 0);
> +       for (int i = 0; i < argc; i++) {
> +               error("unknown parameter \"%s\"\n", argv[i]);
> +       }
> +       if (argc)
> +               usage_with_options(u, options);
> +
> +       ndctl_set_log_fn((struct ndctl_ctx *)ctx, logreport);
> +       ndctl_set_log_priority((struct ndctl_ctx *)ctx, LOG_INFO);
> +
> +       if (read_config_file((struct ndctl_ctx *)ctx, prefix))
> +               goto out;
> +
> +       if (monitor.logfile) {
> +               if (strcmp(monitor.logfile, "./stderr") == 0)
> +                       log_destination = LOG_DESTINATION_STDERR;
> +               else if (strcmp(monitor.logfile, "./syslog") == 0)
> +                       log_destination = LOG_DESTINATION_SYSLOG;
> +               else
> +                       log_destination = LOG_DESTINATION_FILE;
> +       }
> +
> +       if (monitor.daemon) {
> +               if (daemon(0, 0) != 0) {
> +                       err((struct ndctl_ctx *)ctx, "daemon start failed\n");
> +                       goto out;
> +               }
> +               info((struct ndctl_ctx *)ctx, "ndctl monitor daemon started\n");
> +       }
> +
> +       fctx.filter_bus = filter_bus;
> +       fctx.filter_dimm = filter_dimm;
> +       fctx.filter_region = filter_region;
> +       fctx.filter_namespace = NULL;
> +       fctx.arg = &mfa;
> +       list_head_init(&mfa.mdimm);
> +       mfa.num_dimm = 0;
> +       mfa.maxfd_dimm = -1;
> +       mfa.flags = 0;
> +
> +       if (util_filter_walk(ctx, &fctx, &param))
> +               goto out;
> +
> +       if (!mfa.num_dimm) {
> +               err((struct ndctl_ctx *)ctx, "no dimms can be monitored\n");
> +               goto out;
> +       }
> +
> +       if (monitor_dimm_event(ctx, &mfa))
> +               goto out;
> +
> +       return 0;
> +out:
> +       return 1;
> +}
> diff --git a/ndctl/ndctl.c b/ndctl/ndctl.c
> index 7daadeb..73dabfa 100644
> --- a/ndctl/ndctl.c
> +++ b/ndctl/ndctl.c
> @@ -89,6 +89,7 @@ static struct cmd_struct commands[] = {
>         { "wait-scrub", cmd_wait_scrub },
>         { "start-scrub", cmd_start_scrub },
>         { "list", cmd_list },
> +       { "monitor", cmd_monitor},
>         { "help", cmd_help },
>         #ifdef ENABLE_TEST
>         { "test", cmd_test },
> --
> 2.17.0.140.g0b0cc9f86
>
>
> _______________________________________________
> 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

RE: [PATCH v6 2/4] ndctl, monitor: add ndctl monitor daemon

[Qi, Fuli]

> -----Original Message-----
> From: Dan Williams [mailto:dan.j.williams@intel.com]
> Sent: Saturday, May 12, 2018 3:45 AM
> To: Qi, Fuli/斉 福利 <qi.fuli@jp.fujitsu.com>
> Cc: linux-nvdimm <linux-nvdimm@lists.01.org>
> Subject: Re: [PATCH v6 2/4] ndctl, monitor: add ndctl monitor daemon
> 
> On Sun, May 6, 2018 at 10:09 PM, QI Fuli <qi.fuli@jp.fujitsu.com> wrote:
> > This patch adds the body file of ndctl monitor daemon.
> 
> This is too short. Let's copy your cover letter details into this patch since the cover
> letter is thrown away, but the commit messages are preserved in git:

Thanks for your comments.
I will write more details.

> ---
> 
> ndctl monitor daemon, a tiny daemon to monitor the smart events of nvdimm
> DIMMs. Users can run a monitor as a one-shot command or a daemon in
> background by using the [--daemon] option. DIMMs to monitor can be selected by
> [--dimm] [--bus] [--region] [--namespace] options, these options support multiple
> space-seperated arguments. When a smart event fires, monitor daemon will log the
> notifications which including dimm health status to syslog or a logfile by setting
> [--logfile=<file|syslog>] option. monitor also can output the notifications to stderr
> when it run as one-shot command by setting [--logfile=<stderr>]. The
> notifications follow json format and can be consumed by log collectors like Fluentd.
> Users can change the configuration of monitor by editing the default configuration
> file /etc/ndctl/monitor.conf or by using [--config-file=<file>] option to override the
> default configuration.
> 
> Users can start a monitor daemon by the following command:
>  # ndctl monitor --daemon --logfile /var/log/ndctl/monitor.log
> 
> Also, a monitor daemon can be started by systemd:
>  # systemctl start ndctl-monitor.service In this case, monitor daemon follows the
> default configuration file /etc/ndctl/monitor.conf.
> 
> ---
> 
> However, now that I re-read this description what about the other event types
> (beyond health) on other objects (beyond DIMMs). This should behave like the 'list'
> command where we have filter parameters for devices to monitor *and* event
> types for events to include:
> 
> dimm-events="<list of dimm events>"
> namespace-events="<list of namespace events>"
> region-events="<list of region events>"
> bus-events="<list of bus events>"
> bus="<bus filter option>"
> dimm="<dimm filter option>"
> region="<region filter option>"
> namespace="<namespace filter option>"
> 
> We don't need to support all of this in this first implementation, but see more
> comments below I think there are some changes we can make to start down this
> path.
> 
> >
> > Signed-off-by: QI Fuli <qi.fuli@jp.fujitsu.com>
> > ---
> >  builtin.h         |   1 +
> >  ndctl/Makefile.am |   3 +-
> >  ndctl/monitor.c   | 460
> ++++++++++++++++++++++++++++++++++++++++++++++
> >  ndctl/ndctl.c     |   1 +
> >  4 files changed, 464 insertions(+), 1 deletion(-)  create mode 100644
> > ndctl/monitor.c
> >
> > diff --git a/builtin.h b/builtin.h
> > index d3cc723..675a6ce 100644
> > --- a/builtin.h
> > +++ b/builtin.h
> > @@ -39,6 +39,7 @@ int cmd_inject_error(int argc, const char **argv,
> > void *ctx);  int cmd_wait_scrub(int argc, const char **argv, void
> > *ctx);  int cmd_start_scrub(int argc, const char **argv, void *ctx);
> > int cmd_list(int argc, const char **argv, void *ctx);
> > +int cmd_monitor(int argc, const char **argv, void *ctx);
> >  #ifdef ENABLE_TEST
> >  int cmd_test(int argc, const char **argv, void *ctx);  #endif diff
> > --git a/ndctl/Makefile.am b/ndctl/Makefile.am index d22a379..7dbf223
> > 100644
> > --- a/ndctl/Makefile.am
> > +++ b/ndctl/Makefile.am
> > @@ -16,7 +16,8 @@ ndctl_SOURCES = ndctl.c \
> >                 util/json-smart.c \
> >                 util/json-firmware.c \
> >                 inject-error.c \
> > -               inject-smart.c
> > +               inject-smart.c \
> > +               monitor.c
> >
> >  if ENABLE_DESTRUCTIVE
> >  ndctl_SOURCES += ../test/blk_namespaces.c \ diff --git
> > a/ndctl/monitor.c b/ndctl/monitor.c new file mode 100644 index
> > 0000000..ab6e701
> > --- /dev/null
> > +++ b/ndctl/monitor.c
> > @@ -0,0 +1,460 @@
> > +/*
> > + * Copyright(c) 2018, FUJITSU LIMITED. All rights reserved.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > +modify it
> > + * under the terms and conditions of the GNU Lesser General Public
> > +License,
> > + * version 2.1, as published by the Free Software Foundation.
> 
> Did you intend for this to be LGPL-2.1?
> 
> The licensing we have been doing to date is GPL-2.0 for the utility code (i.e. ndctl/)
> especially because it copies from git which is GPL2.0. LGPL-2.1 is for the library
> routines (i.e. ndctl/lib/). The intent is for applications to be able to use the library
> without needing to share their application source, but improvements to the utility
> are shared back with the project.

I will change it to GPL-2.0.

> > + *
> > + * This program is distributed in the hope it will be useful, but
> > + WITHOUT ANY
> > + * WARRANTY; without even the implied warranty of MERCHANTABILITY or
> > + FITNESS
> > + * FOR A PARTICULAR PURPOSE.  See the GNU Lesser General Public
> > + License for
> > + * more details.
> > + */
> 
> Convert this to SPDX style:
> 
> /* SPDX-License-Identifier: GPL-2.0 */
> /* Copyright(c) 2018, FUJITSU LIMITED. All rights reserved. */
> 
Ok, I see.

> > +
> > +#include <stdio.h>
> > +#include <json-c/json.h>
> > +#include <libgen.h>
> > +#include <dirent.h>
> > +#include <util/log.h>
> > +#include <util/json.h>
> > +#include <util/filter.h>
> > +#include <util/util.h>
> > +#include <util/parse-options.h>
> > +#include <util/strbuf.h>
> > +#include <ndctl/lib/private.h>
> > +#include <ndctl/libndctl.h>
> > +#include <sys/stat.h>
> > +#include <sys/epoll.h>
> > +#define BUF_SIZE 2048
> > +
> > +static enum log_destination {
> > +       LOG_DESTINATION_STDERR = 1,
> > +       LOG_DESTINATION_SYSLOG = 2,
> > +       LOG_DESTINATION_FILE = 3,
> > +} log_destination = LOG_DESTINATION_SYSLOG;
> > +
> > +static struct {
> > +       const char *logfile;
> > +       const char *config_file;
> > +       bool daemon;
> > +} monitor;
> > +
> > +struct monitor_dimm_node {
> > +       struct ndctl_dimm *dimm;
> > +       int health_eventfd;
> > +       struct list_node list;
> > +};
> > +
> > +struct monitor_filter_arg {
> > +       struct list_head mdimm;
> 
> Let's call this field 'dimms' to match the list_head naming style in other parts of the
> code.

OK, I see.
> 
> > +       int maxfd_dimm;
> > +       int num_dimm;
> > +       unsigned long flags;
> > +};
> > +
> > +struct util_filter_params param;
> > +
> > +static int did_fail;
> > +
> > +#define fail(fmt, ...) \
> > +do { \
> > +       did_fail = 1; \
> > +       err(ctx, "ndctl-%s:%s:%d: " fmt, \
> > +                       VERSION, __func__, __LINE__, ##__VA_ARGS__); \
> > +} while (0)
> > +
> > +static bool is_dir(char *filepath)
> > +{
> > +       DIR *dir = opendir(filepath);
> > +       if (dir) {
> > +               closedir(dir);
> > +               return true;
> > +       }
> > +       return false;
> > +}
> > +
> > +static int recur_mkdir(char *filepath, mode_t mode) {
> > +       char *p;
> > +       char *buf = (char *)malloc(strlen(filepath) + 4);
> > +
> > +       strcpy(buf, filepath);
> > +       for (p = strchr(buf + 1, '/'); p; p = strchr(p + 1, '/')) {
> > +               *p = '\0';
> > +               if (!is_dir(buf)) {
> > +                       if (mkdir(buf, mode) < 0) {
> > +                               free(buf);
> > +                               return -1;
> > +                       }
> > +               }
> > +               *p = '/';
> > +       }
> > +
> > +       free(buf);
> > +       return 0;
> > +}
> > +
> > +static void set_value(const char **arg, char *val) {
> > +       struct strbuf value = STRBUF_INIT;
> > +       size_t arg_len = *arg ? strlen(*arg) : 0;
> > +
> > +       if (arg_len) {
> > +               strbuf_add(&value, *arg, arg_len);
> > +               strbuf_addstr(&value, " ");
> > +       }
> > +       strbuf_addstr(&value, val);
> > +       *arg = strbuf_detach(&value, NULL); }
> > +
> > +static void logreport(struct ndctl_ctx *ctx, int priority, const char *file,
> > +               int line, const char *fn, const char *format, va_list
> > +args) {
> > +       char *log_dir;
> > +       char *buf = (char *)malloc(BUF_SIZE);
> > +       vsnprintf(buf, BUF_SIZE, format, args);
> > +
> > +       switch (log_destination) {
> > +       case LOG_DESTINATION_STDERR:
> > +               fprintf(stderr, "%s\n", buf);
> > +               goto end;
> > +
> > +       case LOG_DESTINATION_SYSLOG:
> > +               syslog(priority, "%s\n", buf);
> > +               goto end;
> > +
> > +       case LOG_DESTINATION_FILE:
> > +               log_dir = dirname(strdup(monitor.logfile));
> 
> We leak a memory allocation here, or crash if strdup fails.

Ok, I see.

> > +               if (!is_dir(log_dir) && recur_mkdir(log_dir, 0744) != 0)
> > +                       goto log_file_err;
> 
> No need to create the parent directory structure. Just fail if the parent directories
> are not present. Require the administrator to create the directories ahead of time.
> 
Ok, I will remove the recur_mkdir().
> Actually, I don't see a need to have LOG_DESTINATION_FILE at all. Why not just
> do:
> 
>     ndctl monitor 2>file
> 
> ...to redirect stderr to a file?

In my understanding, this stderr redirection does not make sense when ndctl monitor
runs as a daemon, eg:
   # ndctl monitor --logfile stderr --daemon 2>file
What do you think?
> 
> > +               FILE *f = fopen(monitor.logfile, "a+");
> > +               if (!f)
> > +                       goto log_file_err;
> > +               fprintf(f, "%s\n", buf);
> > +               fclose(f);
> > +               free(log_dir);
> > +               goto end;
> > +
> > +       log_file_err:
> > +               log_destination = LOG_DESTINATION_SYSLOG;
> > +               fail("open logfile %s failed\n%s", monitor.logfile, buf);
> > +               free(log_dir);
> > +       }
> > +end:
> > +       free(buf);
> > +       return;
> > +}
> > +
> > +static void notify_json_msg(struct ndctl_ctx *ctx, struct ndctl_dimm
> > +*dimm) {
> > +       struct json_object *jmsg, *jdatetime, *jpid, *jdimm, *jhealth;
> > +       struct timespec ts;
> > +       char datetime[32];
> > +
> > +       jmsg = json_object_new_object();
> > +       if (!jmsg) {
> > +               fail("\n");
> > +               return;
> > +       }
> > +
> > +       clock_gettime(CLOCK_REALTIME, &ts);
> > +       sprintf(datetime, "%10ld.%09ld", ts.tv_sec, ts.tv_nsec);
> > +       jdatetime = json_object_new_string(datetime);
> > +       if (!jdatetime) {
> > +               fail("\n");
> > +               return;
> > +       }
> > +       json_object_object_add(jmsg, "datetime", jdatetime);
> 
> Let's call this field 'timestamp'.

Ok, I see.
> > +
> > +       jpid = json_object_new_int((int)getpid());
> 
> No need to cast here.

Ok, I see.
> 
> > +       if (!jpid) {
> > +               fail("\n");
> > +               return;
> > +       }
> > +       json_object_object_add(jmsg, "pid", jpid);
> > +
> > +       jdimm = util_dimm_to_json(dimm, 0);
> > +       if (!dimm) {
> > +               fail("\n");
> > +               return;
> > +       }
> > +       json_object_object_add(jmsg, "dimm", jdimm);
> > +
> > +       jhealth = util_dimm_health_to_json(dimm);
> > +       if (!jhealth) {
> > +               fail("\n");
> > +               return;
> > +       }
> > +       json_object_object_add(jdimm, "health", jhealth);
> > +
> > +       notice(ctx, "%s",
> > +               json_object_to_json_string_ext(jmsg,
> > + JSON_C_TO_STRING_PLAIN));
> 
> As mentioned above this function seems to assume that the only DIMM events to
> send are DIMM health events. It's ok to save other object monitoring to a later patch,
> but let's at least support DIMM health
> events:
> 
> dimm-spares-remaining
> dimm-media-temperature
> dimm-controller-temperature
> dimm-health-state
> 
> ...and DIMM detected events:
> 
> dimm-unclean-shutdown
> dimm-detected
> 
> There should be an event type included in the json. Along with 'timestamp' and 'pid'
> I think we need an 'event' field so that consumer code can make assumptions about
> the format of the event record. I think 'dimm-health' and 'dimm-detect' are the only
> event record types we need to support in this initial version.
>  

Ok, I will add events come DIMMs in the first implementation.

> > +}
> > +
> > +static bool filter_region(struct ndctl_region *region,
> > +               struct util_filter_ctx *ctx) {
> > +       return true;
> > +}
> > +
> > +static void filter_dimm(struct ndctl_dimm *dimm, struct
> > +util_filter_ctx *ctx) {
> > +       struct monitor_filter_arg *mfa = (struct monitor_filter_arg
> > +*)ctx->arg;
> > +
> > +       if (!ndctl_dimm_is_cmd_supported(dimm,
> ND_CMD_SMART_THRESHOLD))
> > +               return;
> 
> Similar to above this assumes only health events, let's make the code more generic.
> 
Ok, I will change it.
> > +
> > +       struct monitor_dimm_node *mdn = malloc(sizeof(*mdn));
> > +       mdn->dimm = dimm;
> > +       int fd = ndctl_dimm_get_health_eventfd(dimm);
> 
> Let's not declare new variables in the middle of the function. I'll go add
> "-Wdeclaration-after-statement" to the default warning flags.

Ok, I see.
> 
> > +       mdn->health_eventfd = fd;
> > +       list_add_tail(&mfa->mdimm, &mdn->list);
> > +       if (fd > mfa->maxfd_dimm)
> > +               mfa->maxfd_dimm = fd;
> > +       mfa->num_dimm++;
> > +}
> > +
> > +static bool filter_bus(struct ndctl_bus *bus, struct util_filter_ctx
> > +*ctx) {
> > +       return true;
> > +}
> > +
> > +static int monitor_dimm_event(struct ndctl_ctx *ctx,
> > +               struct monitor_filter_arg *mfa) {
> > +       struct epoll_event ev, events[mfa->num_dimm];
> 
> Allocate events with malloc rather than a VLA.

Ok, I see.
> 
> > +       struct ndctl_dimm **dimms;
> > +       int nfds, epollfd;
> > +       struct monitor_dimm_node *mdn;
> > +       char buf;
> > +
> > +       dimms = calloc(mfa->maxfd_dimm + 1, sizeof(struct ndctl_dimm
> > + *));
> 
> Why a separate allocation? I believe you can store pointers back to the 'dimm'
> objects in the epoll_event data.

Ok, I will refactor it.
> 
> > +       if (!dimms) {
> > +               fail("\n");
> > +               goto out;
> > +       }
> > +
> > +       epollfd = epoll_create1(0);
> > +       if (epollfd == -1) {
> > +               err(ctx, "epoll_create1 error\n");
> > +               goto out;
> > +       }
> > +       list_for_each(&mfa->mdimm, mdn, list) {
> > +               memset(&ev, 0, sizeof(ev));
> 
> Why memset?

I will remove it.
> 
> > +               pread(mdn->health_eventfd, &buf, 1, 0);
> > +               ev.data.fd = mdn->health_eventfd;
> > +               if (epoll_ctl(epollfd, EPOLL_CTL_ADD,
> > +                               mdn->health_eventfd, &ev) != 0) {
> > +                       err(ctx, "epoll_ctl error\n");
> > +                       goto out;
> > +               }
> > +               dimms[mdn->health_eventfd] = mdn->dimm;
> > +       }
> > +
> > +       while(1){
> 
> Follow kernel coding style, i.e. add spaces: "while (1) {"

Ok, I see.
> 
> > +               nfds = epoll_wait(epollfd, events, mfa->num_dimm, -1);
> > +               if (nfds <= 0) {
> > +                       err(ctx, "epoll_wait error\n");
> > +                       goto out;
> > +               }
> > +               for (int i = 0; i < nfds; i++) {
> 
> Move declaration of 'i' to the top of the function.

Ok, I see.
> 
> > +                       memset(&ev, 0, sizeof(ev));
> 
> Why memset?

The same I will remove it.
> 
> > +                       ev = events[i];
> > +                       notify_json_msg(ctx, dimms[ev.data.fd]);
> > +                       pread(ev.data.fd, &buf, 1, 0);
> > +               }
> > +               if (did_fail)
> > +                       goto out;
> > +       }
> > +       return 0;
> > +out:
> > +       free(dimms);
> > +       return 1;
> > +}
> > +
> > +static int read_config_file(struct ndctl_ctx *ctx, const char
> > +*prefix)
> 
> This should move to its own source file in util/.
> 
> > +{
> > +       FILE *f;
> > +       int line_nr = 0;
> > +       char buf[BUF_SIZE];
> > +       char *config_file = "/etc/ndctl/monitor.conf";
> > +
> > +       if (monitor.config_file)
> > +               f = fopen(monitor.config_file, "r");
> > +       else
> > +               f = fopen(config_file, "r");
> > +
> > +       if (f == NULL) {
> > +               error("config-file: %s cannot be opened\n", config_file);
> > +               return -1;
> > +       }
> > +
> > +       while (fgets(buf, BUF_SIZE, f)) {
> > +               size_t len;
> > +               char *key;
> > +               char *val;
> > +
> > +               line_nr++;
> > +
> > +               /* find key */
> > +               key = buf;
> > +               while (isspace(key[0]))
> > +                       key++;
> > +
> > +               /* comment or empty line */
> > +               if (key[0] == '#' || key[0] == '\0')
> > +                       continue;
> > +
> > +               /* split key/value */
> > +               val = strchr(key, '=');
> > +               if (!val) {
> > +                       err(ctx, "missing <key>=<value> in '%s'[%i], skip
> line\n",
> > +                                       config_file, line_nr);
> > +                       continue;
> > +               }
> > +
> > +               val[0] = '\0';
> > +               val++;
> > +
> > +               /* find value */
> > +               while (isspace(val[0]))
> > +                       val++;
> > +
> > +               /* terminate key */
> > +               len = strlen(key);
> > +               if (len == 0)
> > +                       continue;
> > +               while (isspace(key[len-1]))
> > +                       len--;
> > +               key[len] = '\0';
> > +
> > +               /*terminate value */
> > +               len = strlen(val);
> > +               if (len == 0)
> > +                       continue;
> > +               while (isspace(val[len-1]))
> > +                       len--;
> > +               val[len] = '\0';
> > +
> > +               if (len == 0)
> > +                       continue;
> > +
> > +               /* unquote */
> > +               if (val[0] == '"' || val[0] == '\'') {
> > +                       if (val[len-1] != val[0]) {
> > +                               err(ctx, "inconsistent quoting in '%s'[%i], skip
> line\n",
> > +                                               config_file, line_nr);
> > +                               continue;
> > +                       }
> > +                       val[len-1] = '\0';
> > +                       val++;
> > +               }
> > +
> > +               if (strcmp(key, "bus") == 0) {
> > +                       set_value((const char **)&param.bus, val);
> > +                       continue;
> > +               }
> > +               if (strcmp(key, "dimm") == 0) {
> > +                       set_value((const char **)&param.dimm, val);
> > +                       continue;
> > +               }
> > +               if (strcmp(key, "region") == 0) {
> > +                       set_value((const char **)&param.region, val);
> > +                       continue;
> > +               }
> > +               if (strcmp(key, "namespace") == 0) {
> > +                       set_value((const char **)&param.namespace, val);
> > +                       continue;
> > +               }
> > +               if (strcmp(key, "logfile") == 0) {
> > +                       if (monitor.logfile)
> > +                               continue;
> > +                       set_value((const char **)&monitor.logfile, val);
> > +                       fix_filename(prefix, (const char **)&monitor.logfile);
> > +               }
> > +       }
> > +       fclose(f);
> > +       return 0;
> 
> Given that you said you borrowed this configuration file format from udev, can we
> borrow their parsing code as well. I'd rather not carry custom parsing code in ndctl.

Think more about this implementation. Since the License of src/libudev.c which I borrowed
part of read_config_file() from is LGPL-2.1, I will rewrite it to avoid license issue and just
use the original codes as a reference
> 
> > +}
> > +
> > +int cmd_monitor(int argc, const char **argv, void *ctx) {
> > +       const struct option options[] = {
> > +               OPT_STRING('b', "bus", &param.bus, "bus-id", "filter by bus"),
> > +               OPT_STRING('r', "region", &param.region, "region-id",
> > +                               "filter by region"),
> > +               OPT_STRING('d', "dimm", &param.dimm, "dimm-id",
> > +                               "filter by dimm"),
> > +               OPT_STRING('n', "namespace", &param.namespace,
> > +                               "namespace-id", "filter by namespace id"),
> > +               OPT_FILENAME('l', "logfile", &monitor.logfile, "file|syslog|stderr",
> > +                               "the place to output the monitor's notification"),
> > +               OPT_FILENAME('c',"config-file", &monitor.config_file,
> "config-file",
> > +                               "use file to override the default configuration"),
> > +               OPT_BOOLEAN('D',"daemon", &monitor.daemon,
> > +                               "run ndctl monitor as a daemon"),
> > +               OPT_END(),
> > +       };
> > +       const char * const u[] = {
> > +               "ndctl monitor [<options>]",
> > +               NULL
> > +       };
> > +       const char *prefix = "./";
> > +       struct util_filter_ctx fctx = { 0 };
> > +       struct monitor_filter_arg mfa = { 0 };
> > +
> > +       argc = parse_options_prefix(argc, argv, prefix, options, u, 0);
> > +       for (int i = 0; i < argc; i++) {
> > +               error("unknown parameter \"%s\"\n", argv[i]);
> > +       }
> > +       if (argc)
> > +               usage_with_options(u, options);
> > +
> > +       ndctl_set_log_fn((struct ndctl_ctx *)ctx, logreport);
> > +       ndctl_set_log_priority((struct ndctl_ctx *)ctx, LOG_INFO);
> > +
> > +       if (read_config_file((struct ndctl_ctx *)ctx, prefix))
> > +               goto out;
> > +
> > +       if (monitor.logfile) {
> > +               if (strcmp(monitor.logfile, "./stderr") == 0)
> > +                       log_destination = LOG_DESTINATION_STDERR;
> > +               else if (strcmp(monitor.logfile, "./syslog") == 0)
> > +                       log_destination = LOG_DESTINATION_SYSLOG;
> > +               else
> > +                       log_destination = LOG_DESTINATION_FILE;
> > +       }
> > +
> > +       if (monitor.daemon) {
> > +               if (daemon(0, 0) != 0) {
> > +                       err((struct ndctl_ctx *)ctx, "daemon start failed\n");
> > +                       goto out;
> > +               }
> > +               info((struct ndctl_ctx *)ctx, "ndctl monitor daemon started\n");
> > +       }
> > +
> > +       fctx.filter_bus = filter_bus;
> > +       fctx.filter_dimm = filter_dimm;
> > +       fctx.filter_region = filter_region;
> > +       fctx.filter_namespace = NULL;
> > +       fctx.arg = &mfa;
> > +       list_head_init(&mfa.mdimm);
> > +       mfa.num_dimm = 0;
> > +       mfa.maxfd_dimm = -1;
> > +       mfa.flags = 0;
> > +
> > +       if (util_filter_walk(ctx, &fctx, &param))
> > +               goto out;
> > +
> > +       if (!mfa.num_dimm) {
> > +               err((struct ndctl_ctx *)ctx, "no dimms can be monitored\n");
> > +               goto out;
> > +       }
> > +
> > +       if (monitor_dimm_event(ctx, &mfa))
> > +               goto out;
> > +
> > +       return 0;
> > +out:
> > +       return 1;
> > +}
> > diff --git a/ndctl/ndctl.c b/ndctl/ndctl.c index 7daadeb..73dabfa
> > 100644
> > --- a/ndctl/ndctl.c
> > +++ b/ndctl/ndctl.c
> > @@ -89,6 +89,7 @@ static struct cmd_struct commands[] = {
> >         { "wait-scrub", cmd_wait_scrub },
> >         { "start-scrub", cmd_start_scrub },
> >         { "list", cmd_list },
> > +       { "monitor", cmd_monitor},
> >         { "help", cmd_help },
> >         #ifdef ENABLE_TEST
> >         { "test", cmd_test },
> > --
> > 2.17.0.140.g0b0cc9f86
> >
> >
> > _______________________________________________
> > 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

Re: [PATCH v6 2/4] ndctl, monitor: add ndctl monitor daemon

[Dan Williams]

On Tue, May 15, 2018 at 1:32 AM, Qi, Fuli <qi.fuli@jp.fujitsu.com> wrote:
[..]
>> Actually, I don't see a need to have LOG_DESTINATION_FILE at all. Why not just
>> do:
>>
>>     ndctl monitor 2>file
>>
>> ...to redirect stderr to a file?
>
> In my understanding, this stderr redirection does not make sense when ndctl monitor
> runs as a daemon, eg:
>    # ndctl monitor --logfile stderr --daemon 2>file
> What do you think?

True, and now that I look dnsmasq allows the same with its
--log-facility option. Ok, let's keep this feature.

I appreciate the continued effort and patience.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

RE: [PATCH v6 2/4] ndctl, monitor: add ndctl monitor daemon

[Qi, Fuli]

> -----Original Message-----
> From: Dan Williams [mailto:dan.j.williams@intel.com]
> Sent: Wednesday, May 16, 2018 2:07 AM
> To: Qi, Fuli/斉 福利 <qi.fuli@jp.fujitsu.com>
> Cc: linux-nvdimm <linux-nvdimm@lists.01.org>
> Subject: Re: [PATCH v6 2/4] ndctl, monitor: add ndctl monitor daemon
> 
> On Tue, May 15, 2018 at 1:32 AM, Qi, Fuli <qi.fuli@jp.fujitsu.com> wrote:
> [..]
> >> Actually, I don't see a need to have LOG_DESTINATION_FILE at all. Why
> >> not just
> >> do:
> >>
> >>     ndctl monitor 2>file
> >>
> >> ...to redirect stderr to a file?
> >
> > In my understanding, this stderr redirection does not make sense when
> > ndctl monitor runs as a daemon, eg:
> >    # ndctl monitor --logfile stderr --daemon 2>file What do you think?
> 
> True, and now that I look dnsmasq allows the same with its --log-facility option. Ok,
> let's keep this feature.
> 
> I appreciate the continued effort and patience.
> 
Ok, I see.
Thank you very much.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

RE: [PATCH v6 2/4] ndctl, monitor: add ndctl monitor daemon

[Qi, Fuli]

> As mentioned above this function seems to assume that the only DIMM events to
> send are DIMM health events. It's ok to save other object monitoring to a later patch,
> but let's at least support DIMM health
> events:
> 
> dimm-spares-remaining
> dimm-media-temperature
> dimm-controller-temperature
> dimm-health-state
> 
> ...and DIMM detected events:
> 
> dimm-unclean-shutdown
> dimm-detected
> 
> There should be an event type included in the json. Along with 'timestamp' and 'pid'
> I think we need an 'event' field so that consumer code can make assumptions about
> the format of the event record. I think 'dimm-health' and 'dimm-detect' are the only
> event record types we need to support in this initial version.
> 

Hi, Dan

I would like to confirm whether my understanding of the feature in each dimm-event is right or not.
 a) dimm-spares-remaining	
   Checking the Spare Block Remaining Trip in Alarm Trips, if set then the notification is dimm-spares-remaining.
 b) dimm-media-temperature
   Checking the NVDIMM Media Temperature Trip in Alarm Trips, if set then the notification is dimm-media-temperature.
 c) dimm-controller-temperature
   Checking the NVDIMM Controller Temperature Trip in Alarm Trips, if set then the notification is dimm-controller-temperature.
 d) dimm-health-state
   Checking the Health Status, if changed then the notification is dimm-health-state.
 e) dimm-unclean-shutdown
   Checking the Last Shutdown Status, if changed then the notification is dimm-unclean-shutdown.
 f) dimm-detected
   Checking the UUID of DIMM, if changed then the notification is dimm-detected.

Is there a possibility that a notification contains multiple dimm-events?

Should I need to turn off the event alarm after the notification logged?

Thank you very much.
 QI
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v6 2/4] ndctl, monitor: add ndctl monitor daemon

[Dan Williams]

On Fri, May 25, 2018 at 1:49 AM, Qi, Fuli <qi.fuli@jp.fujitsu.com> wrote:
>> As mentioned above this function seems to assume that the only DIMM events to
>> send are DIMM health events. It's ok to save other object monitoring to a later patch,
>> but let's at least support DIMM health
>> events:
>>
>> dimm-spares-remaining
>> dimm-media-temperature
>> dimm-controller-temperature
>> dimm-health-state
>>
>> ...and DIMM detected events:
>>
>> dimm-unclean-shutdown
>> dimm-detected
>>
>> There should be an event type included in the json. Along with 'timestamp' and 'pid'
>> I think we need an 'event' field so that consumer code can make assumptions about
>> the format of the event record. I think 'dimm-health' and 'dimm-detect' are the only
>> event record types we need to support in this initial version.
>>
>
> Hi, Dan
>
> I would like to confirm whether my understanding of the feature in each dimm-event is right or not.
>  a) dimm-spares-remaining
>    Checking the Spare Block Remaining Trip in Alarm Trips, if set then the notification is dimm-spares-remaining.

Yes.

>  b) dimm-media-temperature
>    Checking the NVDIMM Media Temperature Trip in Alarm Trips, if set then the notification is dimm-media-temperature.

Yes.

>  c) dimm-controller-temperature
>    Checking the NVDIMM Controller Temperature Trip in Alarm Trips, if set then the notification is dimm-controller-temperature.

Yes.

>  d) dimm-health-state
>    Checking the Health Status, if changed then the notification is dimm-health-state.

Yes.

>  e) dimm-unclean-shutdown
>    Checking the Last Shutdown Status, if changed then the notification is dimm-unclean-shutdown.

Yes.

>  f) dimm-detected
>    Checking the UUID of DIMM, if changed then the notification is dimm-detected.

No, this would fire for each DIMM detected when the daemon starts up,
and for any future DIMM that is hot plugged into the system. The
notification of hotplugged DIMM devices would be a uevent. We can save
this notification for later as it is different from the
'health_event_fd' that these other notifications are communicated.

> Is there a possibility that a notification contains multiple dimm-events?

Yes, we may only get 1 notification from the kernel, but all of these
items might have changed / tripped.

>
> Should I need to turn off the event alarm after the notification logged?

No, if the kernel continues to send events then the monitor should log them.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH v6 3/4] ndctl, monitor: add default configuration file

[QI Fuli]

This patch adds the default configuration file of ndctl monitor.
Users can change the configuration by editing this file or by
using [--config-file=<file>] option to override this one.
The monitor started by systemd follows the configuration in this file.

Signed-off-by: QI Fuli <qi.fuli@jp.fujitsu.com>
---
 ndctl/Makefile.am  |  5 +++++
 ndctl/monitor.conf | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+)
 create mode 100644 ndctl/monitor.conf

diff --git a/ndctl/Makefile.am b/ndctl/Makefile.am
index 7dbf223..ae3d894 100644
--- a/ndctl/Makefile.am
+++ b/ndctl/Makefile.am
@@ -42,3 +42,8 @@ ndctl_SOURCES += ../test/libndctl.c \
 		 ../test/multi-pmem.c \
 		 ../test/core.c
 endif
+
+monitor_config_file = monitor.conf
+monitor_configdir = /etc/ndctl/
+monitor_config_DATA = $(monitor_config_file)
+EXTRA_DIST += $(monitor_config_file)
diff --git a/ndctl/monitor.conf b/ndctl/monitor.conf
new file mode 100644
index 0000000..13ad7b0
--- /dev/null
+++ b/ndctl/monitor.conf
@@ -0,0 +1,37 @@
+# This is the main ndctl monitor configuration file. It contains the
+# configuration directives that give ndctl monitor instructions.
+# You can change the configuration of ndctl monitor by editing this
+# file or by using [--config-file=<file>] option to override this one.
+# The changed value will work after restart ndctl monitor service.
+
+# In this file, lines starting with a hash (#) are comments.
+# The configurations shold follow <key> = <value> style.
+# Multiple space-seperated values are allowed, but except the following
+# charecters: : # ? / \ % " '
+
+# The dimms to monitor are filtered via dimm's name by setting key "dimm".
+# If this value is different from the value of [--dimm=<value>] option,
+# both of the values will work.
+# dimm = all
+
+# The dimms to monitor are filtered via its parent bus by setting key "bus".
+# If this value is different from the value of [--bus=<value>] option,
+# both of the values will work.
+# bus = all
+
+# The dimms to monitor are filtered via region by setting key "region".
+# If this value is different from the value of [--namespace=<value>] option,
+# both of the values will work.
+# region = all
+
+# The dimms to monitor are filtered via namespace by setting key "namespace".
+# If this value is different from the value of [--namespace=<value>] option,
+# both of the values will work.
+# namespace = all
+
+# Users can choose to output the notifications to syslog (logfile=syslog),
+# stderr (logfile=stderr) or to write into a special file (logfile=<file>)
+# by setting key "logfile".
+# If this value is in conflict with the value of [--logfile=<value>] option,
+# this value will be ignored.
+# logfile = /var/log/ndctl/monitor.log
-- 
2.17.0.140.g0b0cc9f86


_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH v6 4/4] ndctl, monitor: add the unit file of systemd for ndctl-monitor service

[QI Fuli]

This patch adds the systemd unit file for ndctl-monitor service.
The systemd unit directory can be configured by setting environment
variable "--with-systemd-unit-dir[=DIR]".

Signed-off-by: QI Fuli <qi.fuli@jp.fujitsu.com>
---
 autogen.sh                  |  3 ++-
 configure.ac                | 22 ++++++++++++++++++++++
 ndctl/Makefile.am           |  4 ++++
 ndctl/ndctl-monitor.service |  7 +++++++
 4 files changed, 35 insertions(+), 1 deletion(-)
 create mode 100644 ndctl/ndctl-monitor.service

diff --git a/autogen.sh b/autogen.sh
index a23cf53..b226c7a 100755
--- a/autogen.sh
+++ b/autogen.sh
@@ -17,7 +17,8 @@ libdir() {
 
 args="--prefix=/usr \
 --sysconfdir=/etc \
---libdir=$(libdir /usr/lib)"
+--libdir=$(libdir /usr/lib) \
+--with-systemd-unit-dir"
 
 echo
 echo "----------------------------------------------------------------"
diff --git a/configure.ac b/configure.ac
index cddad16..fd227eb 100644
--- a/configure.ac
+++ b/configure.ac
@@ -135,6 +135,27 @@ AC_CHECK_FUNCS([ \
 	secure_getenv\
 ])
 
+PKG_PROG_PKG_CONFIG
+AC_ARG_WITH([systemd-unit-dir],
+	AS_HELP_STRING([--with-systemd-unit-dir[=DIR]],
+		[Directory for systemd service files]),
+	[],
+	[with_systemd_unit_dir=yes])
+
+if test "x$with_systemd_unit_dir" = "xyes"; then
+	def_systemd_unit_dir=$($PKG_CONFIG --variable=systemdsystemunitdir systemd)
+	if test "x$def_systemd_unit_dir" = "x"; then
+		AC_MSG_ERROR([systemd support requested but pkg-config unable to query systemd package])
+		with_systemd_unit_dir=no
+	else
+		with_systemd_unit_dir="$def_systemd_unit_dir"
+	fi
+fi
+
+AS_IF([test "x$with_systemd_unit_dir" != "xno"],
+	[AC_SUBST([systemd_unitdir], [$with_systemd_unit_dir])])
+AM_CONDITIONAL([ENABLE_SYSTEMD_UNIT_DIR], [test "x$with_systemd_unit_dir" != "xno"])
+
 my_CFLAGS="\
 -Wall \
 -Wchar-subscripts \
@@ -172,6 +193,7 @@ AC_MSG_RESULT([
         sysconfdir:             ${sysconfdir}
         libdir:                 ${libdir}
         includedir:             ${includedir}
+	systemd-unit-dir:	${systemd_unitdir}
 
         compiler:               ${CC}
         cflags:                 ${CFLAGS}
diff --git a/ndctl/Makefile.am b/ndctl/Makefile.am
index ae3d894..9d008d5 100644
--- a/ndctl/Makefile.am
+++ b/ndctl/Makefile.am
@@ -47,3 +47,7 @@ monitor_config_file = monitor.conf
 monitor_configdir = /etc/ndctl/
 monitor_config_DATA = $(monitor_config_file)
 EXTRA_DIST += $(monitor_config_file)
+
+if ENABLE_SYSTEMD_UNIT_DIR
+systemd_unit_DATA = ndctl-monitor.service
+endif
diff --git a/ndctl/ndctl-monitor.service b/ndctl/ndctl-monitor.service
new file mode 100644
index 0000000..44f9326
--- /dev/null
+++ b/ndctl/ndctl-monitor.service
@@ -0,0 +1,7 @@
+[Unit]
+Description=Ndctl Monitor Daemon
+
+[Service]
+Type=forking
+ExecStart=/usr/bin/ndctl monitor --daemon
+ExecStop=/bin/kill ${MAINPID}
-- 
2.17.0.140.g0b0cc9f86


_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm