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

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

[QI Fuli]

This is the v5 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 one.

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

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 v4:
 - Adding OPTION_FILENAME to make sure filename is correct
 - Adding configuration file
 - Adding [--config-file] option to override the default cofiguration file
 - 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 dirtctory of systemd unit file to be configurable
 - Changing log_file() and log_syslog to logreport()
 - Changing date format in log 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

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              |  28 +++
 util/help.c                 |   5 -
 util/parse-options.c        |  47 +++-
 util/parse-options.h        |  11 +-
 util/util.h                 |   7 +
 14 files changed, 628 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 v5 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       | 28 ++++++++++++++++++++++++++
 util/help.c          |  5 -----
 util/parse-options.c | 47 ++++++++++++++++++++++++++++++++++++++------
 util/parse-options.h | 11 +++++++++--
 util/util.h          |  7 +++++++
 6 files changed, 87 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..fdf4090
--- /dev/null
+++ b/util/abspath.c
@@ -0,0 +1,28 @@
+/* originally copied from git */
+
+#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

[PATCH v5 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

[PATCH v5 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 of ndctl monitor 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 v5 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..60c6537 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_UNIT_DIR], [$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_UNIT_DIR}
 
         compiler:               ${CC}
         cflags:                 ${CFLAGS}
diff --git a/ndctl/Makefile.am b/ndctl/Makefile.am
index ae3d894..b3053d0 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_unitDATA = 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

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

[Masayoshi Mizuma]

Hello QI,

I think the systemd service is nice, however, it seems that it does
not work fine... because ndctl-monitor.service was not installed
to systemd dir.

On 04/26/2018 07:30 AM, QI Fuli wrote:
> 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..60c6537 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_UNIT_DIR], [$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_UNIT_DIR}
>  
>          compiler:               ${CC}
>          cflags:                 ${CFLAGS}
> diff --git a/ndctl/Makefile.am b/ndctl/Makefile.am
> index ae3d894..b3053d0 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_unitDATA = ndctl-monitor.service
> +endif

I'm not familiar with autoconf syntax, but I think the 'systemd_unitDATA'
may be wrong... *_DATA is correct, isn't it?
And, your autoconf code is a little different from the example of autoconf
guide for systemd [1]. It might be a good idea to use the example code simply
because it makes easy to find the systemd code for someone who try to
add systemd service in the future.

[1] https://www.freedesktop.org/software/systemd/man/daemon.html

> 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}
> 

How about the following change?

---
 autogen.sh                  |  3 ++-
 configure.ac                | 15 +++++++++++++++
 ndctl/Makefile.am           |  4 ++++
 ndctl/ndctl-monitor.service |  7 +++++++
 4 files changed, 28 insertions(+), 1 deletion(-)
 create mode 100644 ndctl/ndctl-monitor.service

diff --git a/autogen.sh b/autogen.sh
index a23cf53..bc58002 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-systemdsystemunitdir"
 
 echo
 echo "----------------------------------------------------------------"
diff --git a/configure.ac b/configure.ac
index 3eaac32..6fe054f 100644
--- a/configure.ac
+++ b/configure.ac
@@ -122,6 +122,20 @@ AC_CHECK_FUNCS([ \
 	secure_getenv\
 ])
 
+AC_ARG_WITH([systemdsystemunitdir],
+	[AS_HELP_STRING([--with-systemdsystemunitdir=DIR], [Directory for systemd service files])],,
+		[with_systemdsystemunitdir=auto])
+AS_IF([test "x$with_systemdsystemunitdir" = "xyes" -o "x$with_systemdsystemunitdir" = "xauto"],
+	[def_systemdsystemunitdir=$($PKG_CONFIG --variable=systemdsystemunitdir systemd)
+	AS_IF([test "x$def_systemdsystemunitdir" = "x"],
+		[AS_IF([test "x$with_systemdsystemunitdir" = "xyes"],
+			[AC_MSG_ERROR([systemd support requested but pkg-config unable to query systemd package])])
+				with_systemdsystemunitdir=no],
+				[with_systemdsystemunitdir="$def_systemdsystemunitdir"])])
+AS_IF([test "x$with_systemdsystemunitdir" != "xno"],
+	[AC_SUBST([systemdsystemunitdir], [$with_systemdsystemunitdir])])
+AM_CONDITIONAL([HAVE_SYSTEMD], [test "x$with_systemdsystemunitdir" != "xno"])
+
 my_CFLAGS="\
 -Wall \
 -Wchar-subscripts \
@@ -159,6 +173,7 @@ AC_MSG_RESULT([
         sysconfdir:             ${sysconfdir}
         libdir:                 ${libdir}
         includedir:             ${includedir}
+        systemdunitdir:         ${systemdsystemunitdir}
 
         compiler:               ${CC}
         cflags:                 ${CFLAGS}
diff --git a/ndctl/Makefile.am b/ndctl/Makefile.am
index ae3d894..6ee4726 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 HAVE_SYSTEMD
+systemdsystemunit_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}
-- 
1.8.3.1

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

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

[Dan Williams]

On Thu, Apr 26, 2018 at 4:30 AM, 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>
>
[..]
> diff --git a/util/abspath.c b/util/abspath.c
> new file mode 100644
> index 0000000..fdf4090
> --- /dev/null
> +++ b/util/abspath.c
> @@ -0,0 +1,28 @@
> +/* originally copied from git */

Lets say the source file name that it came from in and get and add the license.

/* SPDX-License-Identifier: GPL-2.0 */
/* originally copied from git/abspath.c */

Otherwise, this patch looks good to me.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

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

[Qi, Fuli]

> -----Original Message-----
> From: Masayoshi Mizuma [mailto:msys.mizuma@gmail.com]
> Sent: Wednesday, May 2, 2018 12:07 AM
> To: Qi, Fuli/斉 福利 <qi.fuli@jp.fujitsu.com>; linux-nvdimm@lists.01.org
> Subject: Re: [PATCH v5 4/4] ndctl, monitor: add the unit file of systemd for
> ndctl-monitor service
> 
> Hello QI,
> 
> I think the systemd service is nice, however, it seems that it does
> not work fine... because ndctl-monitor.service was not installed
> to systemd dir.
> 
> On 04/26/2018 07:30 AM, QI Fuli wrote:
> > 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..60c6537 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_UNIT_DIR], [$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_UNIT_DIR}
> >
> >          compiler:               ${CC}
> >          cflags:                 ${CFLAGS}
> > diff --git a/ndctl/Makefile.am b/ndctl/Makefile.am
> > index ae3d894..b3053d0 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_unitDATA = ndctl-monitor.service
> > +endif
> 
> I'm not familiar with autoconf syntax, but I think the 'systemd_unitDATA'
> may be wrong... *_DATA is correct, isn't it?

Thank you for your comments.
Yes, this is a bug.
I will make a new version and fix it.

> And, your autoconf code is a little different from the example of autoconf
> guide for systemd [1]. It might be a good idea to use the example code simply
> because it makes easy to find the systemd code for someone who try to
> add systemd service in the future.
> 
> [1] https://www.freedesktop.org/software/systemd/man/daemon.html
> 
> > 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}
> >
> 
> How about the following change?
> 

I did try to use the autoconf systemd example code, but I found the indent of generated code in configure was difficult to read.
Therefore, I didn’t use "AS_IF" macro.
Moreover, the values "auto" and "yes" have the same meaning in this case, I deleted "auto" and make the code easy to read.
Also, imitating the other items in configure.ac like "ENABLE_BASH_COMPLETION", I changed the conditional "HAVE_SYSTEMD" to "ENABLE_SYSTEMD_UNIT_DIR".

> ---
>  autogen.sh                  |  3 ++-
>  configure.ac                | 15 +++++++++++++++
>  ndctl/Makefile.am           |  4 ++++
>  ndctl/ndctl-monitor.service |  7 +++++++
>  4 files changed, 28 insertions(+), 1 deletion(-)
>  create mode 100644 ndctl/ndctl-monitor.service
> 
> diff --git a/autogen.sh b/autogen.sh
> index a23cf53..bc58002 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-systemdsystemunitdir"
> 
>  echo
>  echo "----------------------------------------------------------------"
> diff --git a/configure.ac b/configure.ac
> index 3eaac32..6fe054f 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -122,6 +122,20 @@ AC_CHECK_FUNCS([ \
>  	secure_getenv\
>  ])
> 
> +AC_ARG_WITH([systemdsystemunitdir],
> +	[AS_HELP_STRING([--with-systemdsystemunitdir=DIR], [Directory for
> systemd service files])],,
> +		[with_systemdsystemunitdir=auto])
> +AS_IF([test "x$with_systemdsystemunitdir" = "xyes" -o
> "x$with_systemdsystemunitdir" = "xauto"],
> +	[def_systemdsystemunitdir=$($PKG_CONFIG
> --variable=systemdsystemunitdir systemd)
> +	AS_IF([test "x$def_systemdsystemunitdir" = "x"],
> +		[AS_IF([test "x$with_systemdsystemunitdir" = "xyes"],
> +			[AC_MSG_ERROR([systemd support requested but
> pkg-config unable to query systemd package])])
> +				with_systemdsystemunitdir=no],
> +
> 	[with_systemdsystemunitdir="$def_systemdsystemunitdir"])])
> +AS_IF([test "x$with_systemdsystemunitdir" != "xno"],
> +	[AC_SUBST([systemdsystemunitdir], [$with_systemdsystemunitdir])])
> +AM_CONDITIONAL([HAVE_SYSTEMD], [test "x$with_systemdsystemunitdir" !=
> "xno"])
> +
>  my_CFLAGS="\
>  -Wall \
>  -Wchar-subscripts \
> @@ -159,6 +173,7 @@ AC_MSG_RESULT([
>          sysconfdir:             ${sysconfdir}
>          libdir:                 ${libdir}
>          includedir:             ${includedir}
> +        systemdunitdir:         ${systemdsystemunitdir}
> 
>          compiler:               ${CC}
>          cflags:                 ${CFLAGS}
> diff --git a/ndctl/Makefile.am b/ndctl/Makefile.am
> index ae3d894..6ee4726 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 HAVE_SYSTEMD
> +systemdsystemunit_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}
> --
> 1.8.3.1
> 
> - Masa

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

RE: [PATCH v5 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: Friday, May 4, 2018 12:26 PM
> To: Qi, Fuli/斉 福利 <qi.fuli@jp.fujitsu.com>
> Cc: linux-nvdimm <linux-nvdimm@lists.01.org>
> Subject: Re: [PATCH v5 1/4] ndctl, util: add OPTION_FILENAME to parse_opt_type
> 
> On Thu, Apr 26, 2018 at 4:30 AM, 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>
> >
> [..]
> > diff --git a/util/abspath.c b/util/abspath.c new file mode 100644
> > index 0000000..fdf4090
> > --- /dev/null
> > +++ b/util/abspath.c
> > @@ -0,0 +1,28 @@
> > +/* originally copied from git */
> 
> Lets say the source file name that it came from in and get and add the license.
> 
> /* SPDX-License-Identifier: GPL-2.0 */
> /* originally copied from git/abspath.c */
> 
> Otherwise, this patch looks good to me.
> 
Thank you for your comment.
I will make a new version and add the license.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm