5:09 a.m.
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(a)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
5:09 a.m.
New subject: [PATCH v6 1/4] ndctl, util: add OPTION_FILENAME to parse_opt_type
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(a)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
4:42 p.m.
New subject: [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(a)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(a)jp.fujitsu.com>
Reviewed-by: Dan Williams <dan.j.williams(a)intel.com>
2:42 a.m.
New subject: [PATCH v6 1/4] ndctl, util: add OPTION_FILENAME to parse_opt_type
-----Original Message-----
From: Dan Williams [mailto:dan.j.williams@intel.com]
Sent: Saturday, May 12, 2018 1:42 AM
To: Qi, Fuli/斉 福利 <qi.fuli(a)jp.fujitsu.com>
Cc: linux-nvdimm <linux-nvdimm(a)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(a)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(a)jp.fujitsu.com>
Reviewed-by: Dan Williams <dan.j.williams(a)intel.com>
Since this patch is independent from the others, could you please merge this one
first?
10:45 p.m.
New subject: [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(a)jp.fujitsu.com>
> Cc: linux-nvdimm <linux-nvdimm(a)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(a)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(a)jp.fujitsu.com>
>
> Reviewed-by: Dan Williams <dan.j.williams(a)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
11:51 p.m.
New subject: [PATCH v6 1/4] ndctl, util: add OPTION_FILENAME to parse_opt_type
-----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(a)intel.com>; Qi, Fuli/斉 福利
<qi.fuli(a)jp.fujitsu.com>
Cc: linux-nvdimm(a)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(a)jp.fujitsu.com>
> > Cc: linux-nvdimm <linux-nvdimm(a)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(a)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(a)jp.fujitsu.com>
> >
> > Reviewed-by: Dan Williams <dan.j.williams(a)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
5:09 a.m.
New subject: [PATCH v6 2/4] ndctl, monitor: add ndctl monitor daemon
This patch adds the body file of ndctl monitor daemon.
Signed-off-by: QI Fuli <qi.fuli(a)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 **)¶m.bus, val);
+ continue;
+ }
+ if (strcmp(key, "dimm") == 0) {
+ set_value((const char **)¶m.dimm, val);
+ continue;
+ }
+ if (strcmp(key, "region") == 0) {
+ set_value((const char **)¶m.region, val);
+ continue;
+ }
+ if (strcmp(key, "namespace") == 0) {
+ set_value((const char **)¶m.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", ¶m.bus, "bus-id",
"filter by bus"),
+ OPT_STRING('r', "region", ¶m.region, "region-id",
+ "filter by region"),
+ OPT_STRING('d', "dimm", ¶m.dimm, "dimm-id",
+ "filter by dimm"),
+ OPT_STRING('n', "namespace", ¶m.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, ¶m))
+ 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
6:45 p.m.
New subject: [PATCH v6 2/4] ndctl, monitor: add ndctl monitor daemon
On Sun, May 6, 2018 at 10:09 PM, QI Fuli <qi.fuli(a)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(a)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 **)¶m.bus, val);
+ continue;
+ }
+ if (strcmp(key, "dimm") == 0) {
+ set_value((const char **)¶m.dimm, val);
+ continue;
+ }
+ if (strcmp(key, "region") == 0) {
+ set_value((const char **)¶m.region, val);
+ continue;
+ }
+ if (strcmp(key, "namespace") == 0) {
+ set_value((const char **)¶m.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", ¶m.bus,
"bus-id", "filter by bus"),
+ OPT_STRING('r', "region", ¶m.region,
"region-id",
+ "filter by region"),
+ OPT_STRING('d', "dimm", ¶m.dimm,
"dimm-id",
+ "filter by dimm"),
+ OPT_STRING('n', "namespace", ¶m.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, ¶m))
+ 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(a)lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
8:32 a.m.
New subject: [PATCH v6 2/4] ndctl, monitor: add ndctl monitor daemon
-----Original Message-----
From: Dan Williams [mailto:dan.j.williams@intel.com]
Sent: Saturday, May 12, 2018 3:45 AM
To: Qi, Fuli/斉 福利 <qi.fuli(a)jp.fujitsu.com>
Cc: linux-nvdimm <linux-nvdimm(a)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(a)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(a)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 **)¶m.bus, val);
> + continue;
> + }
> + if (strcmp(key, "dimm") == 0) {
> + set_value((const char **)¶m.dimm, val);
> + continue;
> + }
> + if (strcmp(key, "region") == 0) {
> + set_value((const char **)¶m.region, val);
> + continue;
> + }
> + if (strcmp(key, "namespace") == 0) {
> + set_value((const char **)¶m.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", ¶m.bus,
"bus-id", "filter by bus"),
> + OPT_STRING('r', "region", ¶m.region,
"region-id",
> + "filter by region"),
> + OPT_STRING('d', "dimm", ¶m.dimm,
"dimm-id",
> + "filter by dimm"),
> + OPT_STRING('n', "namespace",
¶m.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, ¶m))
> + 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(a)lists.01.org
> https://lists.01.org/mailman/listinfo/linux-nvdimm
5:07 p.m.
New subject: [PATCH v6 2/4] ndctl, monitor: add ndctl monitor daemon
On Tue, May 15, 2018 at 1:32 AM, Qi, Fuli <qi.fuli(a)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.
2:44 a.m.
New subject: [PATCH v6 2/4] ndctl, monitor: add ndctl monitor daemon
-----Original Message-----
From: Dan Williams [mailto:dan.j.williams@intel.com]
Sent: Wednesday, May 16, 2018 2:07 AM
To: Qi, Fuli/斉 福利 <qi.fuli(a)jp.fujitsu.com>
Cc: linux-nvdimm <linux-nvdimm(a)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(a)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.
8:49 a.m.
New subject: [PATCH v6 2/4] ndctl, monitor: add ndctl monitor daemon
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
10:23 p.m.
New subject: [PATCH v6 2/4] ndctl, monitor: add ndctl monitor daemon
On Fri, May 25, 2018 at 1:49 AM, Qi, Fuli <qi.fuli(a)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.
5:09 a.m.
New subject: [PATCH v6 3/4] ndctl, monitor: add default configuration file
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(a)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
5:09 a.m.
New subject: [PATCH v6 4/4] ndctl, monitor: add the unit file of systemd for ndctl-monitor service
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(a)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
1195
days inactive
1231
days old
14 comments
4 participants
participants (4)
-
Dan Williams -
QI Fuli -
Qi, Fuli
-
Verma, Vishal L