All of lore.kernel.org
 help / color / mirror / Atom feed
* [ndctl PATCH 0/7] ndctl/monitor: Cleanups and fixes
@ 2018-12-28  3:29 Dan Williams
  2018-12-28  3:30 ` [ndctl PATCH 1/7] ndctl, daxctl: Split builtin.h per-command Dan Williams
                   ` (7 more replies)
  0 siblings, 8 replies; 9+ messages in thread
From: Dan Williams @ 2018-12-28  3:29 UTC (permalink / raw)
  To: linux-nvdimm

Prompted by a need to add more commands to daxctl, and define a new
configuration file for daxctl to install, I took a look at the ndctl
monitor configuration file implementation and several fixes fell out.

An initial attempt to remove casts from the ndctl monitor uncovered
other cleanup opportunities. The motivation for some of the casts in
ndctl/monitor.c was due to the need to de-reference the 'ctx' parameter
passed to the log routines. That issue can be mitigated by teaching the
command harness to pass a typed version of the @ctx argument to the
builtin-command routines. However, looking closer, the monitor should
not be passing @ctx to the log routines, it should establish its own
log-context. That lead to the discovery of a few more cleanup
opportunities, like unnecessary usage of vaprintf().

More is possible. I am not comfortable with the fact that the log
facility dynamically changes the output and the output target based on
the priority. The monitor also has several occasions where it is
dynamically allocating memory unnecessarily.

---

Dan Williams (7):
      ndctl, daxctl: Split builtin.h per-command
      ndctl, daxctl: Add type-safety to command harness
      ndctl/monitor: Drop 'struct ndctl_ctx *' casts
      ndctl/monitor: Unify definition of default monitor configfile path
      ndctl/monitor: Fix / cleanup log_file()
      ndctl/monitor: Drop vasprintf usage
      ndctl/monitor: Kill usage of ndctl/lib/private.h


 builtin.h            |   51 ----------------
 configure.ac         |    8 ++
 daxctl/builtin.h     |    8 ++
 daxctl/daxctl.c      |   16 ++---
 daxctl/list.c        |    2 -
 ndctl/Makefile.am    |    6 +-
 ndctl/bat.c          |    2 -
 ndctl/builtin.h      |   35 +++++++++++
 ndctl/bus.c          |    4 +
 ndctl/create-nfit.c  |    2 -
 ndctl/dimm.c         |   18 +++---
 ndctl/inject-error.c |    3 -
 ndctl/inject-smart.c |    3 -
 ndctl/list.c         |    2 -
 ndctl/monitor.c      |  163 ++++++++++++++++++++------------------------------
 ndctl/namespace.c    |   10 ++-
 ndctl/ndctl.c        |   64 ++++++++++----------
 ndctl/region.c       |    4 +
 ndctl/test.c         |    2 -
 util/main.c          |   13 ++--
 util/main.h          |   20 ++++++
 21 files changed, 207 insertions(+), 229 deletions(-)
 delete mode 100644 builtin.h
 create mode 100644 daxctl/builtin.h
 create mode 100644 ndctl/builtin.h
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [ndctl PATCH 1/7] ndctl, daxctl: Split builtin.h per-command
  2018-12-28  3:29 [ndctl PATCH 0/7] ndctl/monitor: Cleanups and fixes Dan Williams
@ 2018-12-28  3:30 ` Dan Williams
  2018-12-28  3:30 ` [ndctl PATCH 2/7] ndctl, daxctl: Add type-safety to command harness Dan Williams
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Dan Williams @ 2018-12-28  3:30 UTC (permalink / raw)
  To: linux-nvdimm

In preparation for adding more daxctl specific commands, split the
top-level builtin.h into command specific flavors. Leave the common
definitions in the top-level file.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 builtin.h        |   51 ---------------------------------------------------
 daxctl/builtin.h |    6 ++++++
 daxctl/daxctl.c  |    4 +---
 ndctl/builtin.h  |   33 +++++++++++++++++++++++++++++++++
 ndctl/ndctl.c    |    6 +++---
 util/main.c      |    2 +-
 util/main.h      |    7 ++++++-
 7 files changed, 50 insertions(+), 59 deletions(-)
 delete mode 100644 builtin.h
 create mode 100644 daxctl/builtin.h
 create mode 100644 ndctl/builtin.h

diff --git a/builtin.h b/builtin.h
deleted file mode 100644
index 675a6ce79b9c..000000000000
--- a/builtin.h
+++ /dev/null
@@ -1,51 +0,0 @@
-/*
- * Copyright(c) 2015-2017 Intel Corporation. All rights reserved.
- *
- * This program is free software; you can redistribute it and/or modify it
- * under the terms of version 2 of the GNU General Public License as
- * published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful, but
- * WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
- * General Public License for more details.
- */
-#ifndef _NDCTL_BUILTIN_H_
-#define _NDCTL_BUILTIN_H_
-extern const char ndctl_usage_string[];
-extern const char ndctl_more_info_string[];
-
-struct cmd_struct {
-	const char *cmd;
-	int (*fn)(int, const char **, void *ctx);
-};
-
-int cmd_create_nfit(int argc, const char **argv, void *ctx);
-int cmd_enable_namespace(int argc, const char **argv, void *ctx);
-int cmd_create_namespace(int argc, const char **argv, void *ctx);
-int cmd_destroy_namespace(int argc, const char **argv, void *ctx);
-int cmd_disable_namespace(int argc, const char **argv, void *ctx);
-int cmd_check_namespace(int argc, const char **argv, void *ctx);
-int cmd_enable_region(int argc, const char **argv, void *ctx);
-int cmd_disable_region(int argc, const char **argv, void *ctx);
-int cmd_enable_dimm(int argc, const char **argv, void *ctx);
-int cmd_disable_dimm(int argc, const char **argv, void *ctx);
-int cmd_zero_labels(int argc, const char **argv, void *ctx);
-int cmd_read_labels(int argc, const char **argv, void *ctx);
-int cmd_write_labels(int argc, const char **argv, void *ctx);
-int cmd_init_labels(int argc, const char **argv, void *ctx);
-int cmd_check_labels(int argc, const char **argv, void *ctx);
-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
-#ifdef ENABLE_DESTRUCTIVE
-int cmd_bat(int argc, const char **argv, void *ctx);
-#endif
-int cmd_update_firmware(int argc, const char **argv, void *ctx);
-int cmd_inject_smart(int argc, const char **argv, void *ctx);
-#endif /* _NDCTL_BUILTIN_H_ */
diff --git a/daxctl/builtin.h b/daxctl/builtin.h
new file mode 100644
index 000000000000..131a5d86258b
--- /dev/null
+++ b/daxctl/builtin.h
@@ -0,0 +1,6 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright(c) 2015-2018 Intel Corporation. All rights reserved. */
+#ifndef _DAXCTL_BUILTIN_H_
+#define _DAXCTL_BUILTIN_H_
+int cmd_list(int argc, const char **argv, void *ctx);
+#endif /* _DAXCTL_BUILTIN_H_ */
diff --git a/daxctl/daxctl.c b/daxctl/daxctl.c
index 91a4600e262f..545e117a5aad 100644
--- a/daxctl/daxctl.c
+++ b/daxctl/daxctl.c
@@ -28,7 +28,7 @@
 #include <util/strbuf.h>
 #include <util/util.h>
 #include <util/main.h>
-#include <builtin.h>
+#include <daxctl/builtin.h>
 
 const char daxctl_usage_string[] = "daxctl [--version] [--help] COMMAND [ARGS]";
 const char daxctl_more_info_string[] =
@@ -66,8 +66,6 @@ static int cmd_help(int argc, const char **argv, void *ctx)
 	return help_show_man_page(argv[0], "daxctl", "DAXCTL_MAN_VIEWER");
 }
 
-int cmd_list(int argc, const char **argv, void *ctx);
-
 static struct cmd_struct commands[] = {
 	{ "version", cmd_version },
 	{ "list", cmd_list },
diff --git a/ndctl/builtin.h b/ndctl/builtin.h
new file mode 100644
index 000000000000..4ebc50406247
--- /dev/null
+++ b/ndctl/builtin.h
@@ -0,0 +1,33 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright(c) 2015-2018 Intel Corporation. All rights reserved. */
+#ifndef _NDCTL_BUILTIN_H_
+#define _NDCTL_BUILTIN_H_
+int cmd_create_nfit(int argc, const char **argv, void *ctx);
+int cmd_enable_namespace(int argc, const char **argv, void *ctx);
+int cmd_create_namespace(int argc, const char **argv, void *ctx);
+int cmd_destroy_namespace(int argc, const char **argv, void *ctx);
+int cmd_disable_namespace(int argc, const char **argv, void *ctx);
+int cmd_check_namespace(int argc, const char **argv, void *ctx);
+int cmd_enable_region(int argc, const char **argv, void *ctx);
+int cmd_disable_region(int argc, const char **argv, void *ctx);
+int cmd_enable_dimm(int argc, const char **argv, void *ctx);
+int cmd_disable_dimm(int argc, const char **argv, void *ctx);
+int cmd_zero_labels(int argc, const char **argv, void *ctx);
+int cmd_read_labels(int argc, const char **argv, void *ctx);
+int cmd_write_labels(int argc, const char **argv, void *ctx);
+int cmd_init_labels(int argc, const char **argv, void *ctx);
+int cmd_check_labels(int argc, const char **argv, void *ctx);
+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
+#ifdef ENABLE_DESTRUCTIVE
+int cmd_bat(int argc, const char **argv, void *ctx);
+#endif
+int cmd_update_firmware(int argc, const char **argv, void *ctx);
+int cmd_inject_smart(int argc, const char **argv, void *ctx);
+#endif /* _NDCTL_BUILTIN_H_ */
diff --git a/ndctl/ndctl.c b/ndctl/ndctl.c
index 73dabfac3908..9c930b92fe44 100644
--- a/ndctl/ndctl.c
+++ b/ndctl/ndctl.c
@@ -21,7 +21,7 @@
 #include <unistd.h>
 #include <sys/stat.h>
 #include <sys/types.h>
-#include <builtin.h>
+#include <ndctl/builtin.h>
 #include <ndctl/libndctl.h>
 #include <ccan/array_size/array_size.h>
 
@@ -30,8 +30,8 @@
 #include <util/util.h>
 #include <util/main.h>
 
-const char ndctl_usage_string[] = "ndctl [--version] [--help] COMMAND [ARGS]";
-const char ndctl_more_info_string[] =
+static const char ndctl_usage_string[] = "ndctl [--version] [--help] COMMAND [ARGS]";
+static const char ndctl_more_info_string[] =
 	"See 'ndctl help COMMAND' for more information on a specific command.\n"
 	" ndctl --list-cmds to see all available commands";
 
diff --git a/util/main.c b/util/main.c
index 5f35dd9d1c3e..a7992589cfcc 100644
--- a/util/main.c
+++ b/util/main.c
@@ -21,10 +21,10 @@
 #include <unistd.h>
 #include <sys/stat.h>
 #include <sys/types.h>
-#include <builtin.h>
 
 #include <util/strbuf.h>
 #include <util/util.h>
+#include <util/main.h>
 
 int main_handle_options(const char ***argv, int *argc, const char *usage_msg,
 		struct cmd_struct *cmds, int num_cmds)
diff --git a/util/main.h b/util/main.h
index bcfe9f304d6b..d61d10c79630 100644
--- a/util/main.h
+++ b/util/main.h
@@ -16,7 +16,12 @@
 
 #ifndef __MAIN_H__
 #define __MAIN_H__
-struct cmd_struct;
+
+struct cmd_struct {
+	const char *cmd;
+	int (*fn)(int, const char **, void *ctx);
+};
+
 int main_handle_options(const char ***argv, int *argc, const char *usage_msg,
 		struct cmd_struct *cmds, int num_cmds);
 void main_handle_internal_command(int argc, const char **argv, void *ctx,

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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [ndctl PATCH 2/7] ndctl, daxctl: Add type-safety to command harness
  2018-12-28  3:29 [ndctl PATCH 0/7] ndctl/monitor: Cleanups and fixes Dan Williams
  2018-12-28  3:30 ` [ndctl PATCH 1/7] ndctl, daxctl: Split builtin.h per-command Dan Williams
@ 2018-12-28  3:30 ` Dan Williams
  2018-12-28  3:30 ` [ndctl PATCH 3/7] ndctl/monitor: Drop 'struct ndctl_ctx *' casts Dan Williams
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Dan Williams @ 2018-12-28  3:30 UTC (permalink / raw)
  To: linux-nvdimm

A recent attempt to drop casts from ndctl/monitor.c ran afoul of the
'struct btt_chk' usage of an embedded 'struct log_ctx'. It is
generically useful to be able to reuse the log functions and fragile to
force them to be placed at offset zero in the container context.
Instead, just teach the harness the difference between ndctl and daxctl
commands. Let the leaf command implementations receive their expected
context with the correct type.

Reported-by: Vishal Verma <vishal.l.verma@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 daxctl/builtin.h     |    4 +++
 daxctl/daxctl.c      |   12 +++++-----
 daxctl/list.c        |    2 +-
 ndctl/bat.c          |    2 +-
 ndctl/builtin.h      |   50 ++++++++++++++++++++++---------------------
 ndctl/bus.c          |    4 ++-
 ndctl/create-nfit.c  |    2 +-
 ndctl/dimm.c         |   18 ++++++++--------
 ndctl/inject-error.c |    2 +-
 ndctl/inject-smart.c |    2 +-
 ndctl/list.c         |    2 +-
 ndctl/monitor.c      |    2 +-
 ndctl/namespace.c    |   10 ++++-----
 ndctl/ndctl.c        |   58 +++++++++++++++++++++++++-------------------------
 ndctl/region.c       |    4 ++-
 ndctl/test.c         |    2 +-
 util/main.c          |   11 ++++++---
 util/main.h          |   15 +++++++++++--
 18 files changed, 110 insertions(+), 92 deletions(-)

diff --git a/daxctl/builtin.h b/daxctl/builtin.h
index 131a5d86258b..dae2615b7ddb 100644
--- a/daxctl/builtin.h
+++ b/daxctl/builtin.h
@@ -2,5 +2,7 @@
 /* Copyright(c) 2015-2018 Intel Corporation. All rights reserved. */
 #ifndef _DAXCTL_BUILTIN_H_
 #define _DAXCTL_BUILTIN_H_
-int cmd_list(int argc, const char **argv, void *ctx);
+
+struct daxctl_ctx;
+int cmd_list(int argc, const char **argv, struct daxctl_ctx *ctx);
 #endif /* _DAXCTL_BUILTIN_H_ */
diff --git a/daxctl/daxctl.c b/daxctl/daxctl.c
index 545e117a5aad..cb6f50e39170 100644
--- a/daxctl/daxctl.c
+++ b/daxctl/daxctl.c
@@ -35,13 +35,13 @@ const char daxctl_more_info_string[] =
 	"See 'daxctl help COMMAND' for more information on a specific command.\n"
 	" daxctl --list-cmds to see all available commands";
 
-static int cmd_version(int argc, const char **argv, void *ctx)
+static int cmd_version(int argc, const char **argv, struct daxctl_ctx *ctx)
 {
 	printf("%s\n", VERSION);
 	return 0;
 }
 
-static int cmd_help(int argc, const char **argv, void *ctx)
+static int cmd_help(int argc, const char **argv, struct daxctl_ctx *ctx)
 {
 	const char * const builtin_help_subcommands[] = {
 		"list", NULL,
@@ -67,9 +67,9 @@ static int cmd_help(int argc, const char **argv, void *ctx)
 }
 
 static struct cmd_struct commands[] = {
-	{ "version", cmd_version },
-	{ "list", cmd_list },
-	{ "help", cmd_help },
+	{ "version", .d_fn = cmd_version },
+	{ "list", .d_fn = cmd_list },
+	{ "help", .d_fn = cmd_help },
 };
 
 int main(int argc, const char **argv)
@@ -97,7 +97,7 @@ int main(int argc, const char **argv)
 	if (rc)
 		goto out;
 	main_handle_internal_command(argc, argv, ctx, commands,
-			ARRAY_SIZE(commands));
+			ARRAY_SIZE(commands), PROG_DAXCTL);
 	daxctl_unref(ctx);
 	fprintf(stderr, "Unknown command: '%s'\n", argv[0]);
 out:
diff --git a/daxctl/list.c b/daxctl/list.c
index 58c35fafcd4c..e56300ddd95f 100644
--- a/daxctl/list.c
+++ b/daxctl/list.c
@@ -63,7 +63,7 @@ static int num_list_flags(void)
 	return list.regions + list.devs;
 }
 
-int cmd_list(int argc, const char **argv, void *ctx)
+int cmd_list(int argc, const char **argv, struct daxctl_ctx *ctx)
 {
 	const struct option options[] = {
 		OPT_INTEGER('r', "region", &param.region_id, "filter by region"),
diff --git a/ndctl/bat.c b/ndctl/bat.c
index 5e84f6459890..c4496f0aeaa0 100644
--- a/ndctl/bat.c
+++ b/ndctl/bat.c
@@ -16,7 +16,7 @@
 #include <limits.h>
 #include <util/parse-options.h>
 
-int cmd_bat(int argc, const char **argv, void *ctx)
+int cmd_bat(int argc, const char **argv, struct ndctl_ctx *ctx)
 {
 	int loglevel = LOG_DEBUG, i, rc;
 	struct ndctl_test *test;
diff --git a/ndctl/builtin.h b/ndctl/builtin.h
index 4ebc50406247..17300df0403e 100644
--- a/ndctl/builtin.h
+++ b/ndctl/builtin.h
@@ -2,32 +2,34 @@
 /* Copyright(c) 2015-2018 Intel Corporation. All rights reserved. */
 #ifndef _NDCTL_BUILTIN_H_
 #define _NDCTL_BUILTIN_H_
-int cmd_create_nfit(int argc, const char **argv, void *ctx);
-int cmd_enable_namespace(int argc, const char **argv, void *ctx);
-int cmd_create_namespace(int argc, const char **argv, void *ctx);
-int cmd_destroy_namespace(int argc, const char **argv, void *ctx);
-int cmd_disable_namespace(int argc, const char **argv, void *ctx);
-int cmd_check_namespace(int argc, const char **argv, void *ctx);
-int cmd_enable_region(int argc, const char **argv, void *ctx);
-int cmd_disable_region(int argc, const char **argv, void *ctx);
-int cmd_enable_dimm(int argc, const char **argv, void *ctx);
-int cmd_disable_dimm(int argc, const char **argv, void *ctx);
-int cmd_zero_labels(int argc, const char **argv, void *ctx);
-int cmd_read_labels(int argc, const char **argv, void *ctx);
-int cmd_write_labels(int argc, const char **argv, void *ctx);
-int cmd_init_labels(int argc, const char **argv, void *ctx);
-int cmd_check_labels(int argc, const char **argv, void *ctx);
-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);
+
+struct ndctl_ctx;
+int cmd_create_nfit(int argc, const char **argv, struct ndctl_ctx *ctx);
+int cmd_enable_namespace(int argc, const char **argv, struct ndctl_ctx *ctx);
+int cmd_create_namespace(int argc, const char **argv, struct ndctl_ctx *ctx);
+int cmd_destroy_namespace(int argc, const char **argv, struct ndctl_ctx *ctx);
+int cmd_disable_namespace(int argc, const char **argv, struct ndctl_ctx *ctx);
+int cmd_check_namespace(int argc, const char **argv, struct ndctl_ctx *ctx);
+int cmd_enable_region(int argc, const char **argv, struct ndctl_ctx *ctx);
+int cmd_disable_region(int argc, const char **argv, struct ndctl_ctx *ctx);
+int cmd_enable_dimm(int argc, const char **argv, struct ndctl_ctx *ctx);
+int cmd_disable_dimm(int argc, const char **argv, struct ndctl_ctx *ctx);
+int cmd_zero_labels(int argc, const char **argv, struct ndctl_ctx *ctx);
+int cmd_read_labels(int argc, const char **argv, struct ndctl_ctx *ctx);
+int cmd_write_labels(int argc, const char **argv, struct ndctl_ctx *ctx);
+int cmd_init_labels(int argc, const char **argv, struct ndctl_ctx *ctx);
+int cmd_check_labels(int argc, const char **argv, struct ndctl_ctx *ctx);
+int cmd_inject_error(int argc, const char **argv, struct ndctl_ctx *ctx);
+int cmd_wait_scrub(int argc, const char **argv, struct ndctl_ctx *ctx);
+int cmd_start_scrub(int argc, const char **argv, struct ndctl_ctx *ctx);
+int cmd_list(int argc, const char **argv, struct ndctl_ctx *ctx);
+int cmd_monitor(int argc, const char **argv, struct ndctl_ctx *ctx);
 #ifdef ENABLE_TEST
-int cmd_test(int argc, const char **argv, void *ctx);
+int cmd_test(int argc, const char **argv, struct ndctl_ctx *ctx);
 #endif
 #ifdef ENABLE_DESTRUCTIVE
-int cmd_bat(int argc, const char **argv, void *ctx);
+int cmd_bat(int argc, const char **argv, struct ndctl_ctx *ctx);
 #endif
-int cmd_update_firmware(int argc, const char **argv, void *ctx);
-int cmd_inject_smart(int argc, const char **argv, void *ctx);
+int cmd_update_firmware(int argc, const char **argv, struct ndctl_ctx *ctx);
+int cmd_inject_smart(int argc, const char **argv, struct ndctl_ctx *ctx);
 #endif /* _NDCTL_BUILTIN_H_ */
diff --git a/ndctl/bus.c b/ndctl/bus.c
index bcb8c7199152..ce7f76add777 100644
--- a/ndctl/bus.c
+++ b/ndctl/bus.c
@@ -97,7 +97,7 @@ static int bus_action(int argc, const char **argv, const char *usage,
 	return fail ? fail : -ENXIO;
 }
 
-int cmd_start_scrub(int argc, const char **argv, void *ctx)
+int cmd_start_scrub(int argc, const char **argv, struct ndctl_ctx *ctx)
 {
 	char *usage = "ndctl start-scrub [<bus-id> <bus-id2> ... <bus-idN>] [<options>]";
 	int start = bus_action(argc, argv, usage, bus_options,
@@ -112,7 +112,7 @@ int cmd_start_scrub(int argc, const char **argv, void *ctx)
 	}
 }
 
-int cmd_wait_scrub(int argc, const char **argv, void *ctx)
+int cmd_wait_scrub(int argc, const char **argv, struct ndctl_ctx *ctx)
 {
 	char *usage = "ndctl wait-scrub [<bus-id> <bus-id2> ... <bus-idN>] [<options>]";
 	int wait = bus_action(argc, argv, usage, bus_options,
diff --git a/ndctl/create-nfit.c b/ndctl/create-nfit.c
index 6adfae7af014..53319182d8a4 100644
--- a/ndctl/create-nfit.c
+++ b/ndctl/create-nfit.c
@@ -176,7 +176,7 @@ static int write_nfit(struct nfit *nfit, const char *file, int force)
 }
 
 struct ndctl_ctx;
-int cmd_create_nfit(int argc, const char **argv, void *ctx)
+int cmd_create_nfit(int argc, const char **argv, struct ndctl_ctx *ctx)
 {
 	int i, rc = -ENXIO, force = 0;
 	const char * const u[] = {
diff --git a/ndctl/dimm.c b/ndctl/dimm.c
index 699ab57d9faa..c717beeb78ff 100644
--- a/ndctl/dimm.c
+++ b/ndctl/dimm.c
@@ -954,7 +954,7 @@ static const struct option init_options[] = {
 	OPT_END(),
 };
 
-static int dimm_action(int argc, const char **argv, void *ctx,
+static int dimm_action(int argc, const char **argv, struct ndctl_ctx *ctx,
 		int (*action)(struct ndctl_dimm *dimm, struct action_context *actx),
 		const struct option *options, const char *usage)
 {
@@ -1102,7 +1102,7 @@ static int dimm_action(int argc, const char **argv, void *ctx,
 	return rc;
 }
 
-int cmd_write_labels(int argc, const char **argv, void *ctx)
+int cmd_write_labels(int argc, const char **argv, struct ndctl_ctx *ctx)
 {
 	int count = dimm_action(argc, argv, ctx, action_write, write_options,
 			"ndctl write-labels <nmem> [-i <filename>]");
@@ -1112,7 +1112,7 @@ int cmd_write_labels(int argc, const char **argv, void *ctx)
 	return count >= 0 ? 0 : EXIT_FAILURE;
 }
 
-int cmd_read_labels(int argc, const char **argv, void *ctx)
+int cmd_read_labels(int argc, const char **argv, struct ndctl_ctx *ctx)
 {
 	int count = dimm_action(argc, argv, ctx, action_read, read_options,
 			"ndctl read-labels <nmem0> [<nmem1>..<nmemN>] [-o <filename>]");
@@ -1122,7 +1122,7 @@ int cmd_read_labels(int argc, const char **argv, void *ctx)
 	return count >= 0 ? 0 : EXIT_FAILURE;
 }
 
-int cmd_zero_labels(int argc, const char **argv, void *ctx)
+int cmd_zero_labels(int argc, const char **argv, struct ndctl_ctx *ctx)
 {
 	int count = dimm_action(argc, argv, ctx, action_zero, base_options,
 			"ndctl zero-labels <nmem0> [<nmem1>..<nmemN>] [<options>]");
@@ -1132,7 +1132,7 @@ int cmd_zero_labels(int argc, const char **argv, void *ctx)
 	return count >= 0 ? 0 : EXIT_FAILURE;
 }
 
-int cmd_init_labels(int argc, const char **argv, void *ctx)
+int cmd_init_labels(int argc, const char **argv, struct ndctl_ctx *ctx)
 {
 	int count = dimm_action(argc, argv, ctx, action_init, init_options,
 			"ndctl init-labels <nmem0> [<nmem1>..<nmemN>] [<options>]");
@@ -1142,7 +1142,7 @@ int cmd_init_labels(int argc, const char **argv, void *ctx)
 	return count >= 0 ? 0 : EXIT_FAILURE;
 }
 
-int cmd_check_labels(int argc, const char **argv, void *ctx)
+int cmd_check_labels(int argc, const char **argv, struct ndctl_ctx *ctx)
 {
 	int count = dimm_action(argc, argv, ctx, action_check, base_options,
 			"ndctl check-labels <nmem0> [<nmem1>..<nmemN>] [<options>]");
@@ -1152,7 +1152,7 @@ int cmd_check_labels(int argc, const char **argv, void *ctx)
 	return count >= 0 ? 0 : EXIT_FAILURE;
 }
 
-int cmd_disable_dimm(int argc, const char **argv, void *ctx)
+int cmd_disable_dimm(int argc, const char **argv, struct ndctl_ctx *ctx)
 {
 	int count = dimm_action(argc, argv, ctx, action_disable, base_options,
 			"ndctl disable-dimm <nmem0> [<nmem1>..<nmemN>] [<options>]");
@@ -1162,7 +1162,7 @@ int cmd_disable_dimm(int argc, const char **argv, void *ctx)
 	return count >= 0 ? 0 : EXIT_FAILURE;
 }
 
-int cmd_enable_dimm(int argc, const char **argv, void *ctx)
+int cmd_enable_dimm(int argc, const char **argv, struct ndctl_ctx *ctx)
 {
 	int count = dimm_action(argc, argv, ctx, action_enable, base_options,
 			"ndctl enable-dimm <nmem0> [<nmem1>..<nmemN>] [<options>]");
@@ -1172,7 +1172,7 @@ int cmd_enable_dimm(int argc, const char **argv, void *ctx)
 	return count >= 0 ? 0 : EXIT_FAILURE;
 }
 
-int cmd_update_firmware(int argc, const char **argv, void *ctx)
+int cmd_update_firmware(int argc, const char **argv, struct ndctl_ctx *ctx)
 {
 	int count = dimm_action(argc, argv, ctx, action_update, update_options,
 			"ndctl update-firmware <nmem0> [<nmem1>..<nmemN>] [<options>]");
diff --git a/ndctl/inject-error.c b/ndctl/inject-error.c
index 2b2fec0df706..fef6f75716be 100644
--- a/ndctl/inject-error.c
+++ b/ndctl/inject-error.c
@@ -353,7 +353,7 @@ static int do_inject(const char *namespace, struct ndctl_ctx *ctx)
 	return rc;
 }
 
-int cmd_inject_error(int argc, const char **argv, void *ctx)
+int cmd_inject_error(int argc, const char **argv, struct ndctl_ctx *ctx)
 {
 	const char * const u[] = {
 		"ndctl inject-error <namespace> [<options>]",
diff --git a/ndctl/inject-smart.c b/ndctl/inject-smart.c
index d60bbf15801d..7a44a684d630 100644
--- a/ndctl/inject-smart.c
+++ b/ndctl/inject-smart.c
@@ -491,7 +491,7 @@ static int do_smart(const char *dimm_arg, struct ndctl_ctx *ctx)
 	return rc;
 }
 
-int cmd_inject_smart(int argc, const char **argv, void *ctx)
+int cmd_inject_smart(int argc, const char **argv, struct ndctl_ctx *ctx)
 {
 	const char * const u[] = {
 		"ndctl inject-smart <dimm> [<options>]",
diff --git a/ndctl/list.c b/ndctl/list.c
index 33f111fca9d3..506404db11b0 100644
--- a/ndctl/list.c
+++ b/ndctl/list.c
@@ -424,7 +424,7 @@ static int num_list_flags(void)
 	return list.buses + list.dimms + list.regions + list.namespaces;
 }
 
-int cmd_list(int argc, const char **argv, void *ctx)
+int cmd_list(int argc, const char **argv, struct ndctl_ctx *ctx)
 {
 	const struct option options[] = {
 		OPT_STRING('b', "bus", &param.bus, "bus-id", "filter by bus"),
diff --git a/ndctl/monitor.c b/ndctl/monitor.c
index b92a133b7b94..47caae55d56d 100644
--- a/ndctl/monitor.c
+++ b/ndctl/monitor.c
@@ -580,7 +580,7 @@ out:
 	return rc;
 }
 
-int cmd_monitor(int argc, const char **argv, void *ctx)
+int cmd_monitor(int argc, const char **argv, struct ndctl_ctx *ctx)
 {
 	const struct option options[] = {
 		OPT_STRING('b', "bus", &param.bus, "bus-id", "filter by bus"),
diff --git a/ndctl/namespace.c b/ndctl/namespace.c
index ac49fba9d0f4..b35812bd17a9 100644
--- a/ndctl/namespace.c
+++ b/ndctl/namespace.c
@@ -1138,7 +1138,7 @@ static int do_xaction_namespace(const char *namespace,
 	return rc;
 }
 
-int cmd_disable_namespace(int argc, const char **argv, void *ctx)
+int cmd_disable_namespace(int argc, const char **argv, struct ndctl_ctx *ctx)
 {
 	char *xable_usage = "ndctl disable-namespace <namespace> [<options>]";
 	const char *namespace = parse_namespace_options(argc, argv,
@@ -1155,7 +1155,7 @@ int cmd_disable_namespace(int argc, const char **argv, void *ctx)
 	return rc;
 }
 
-int cmd_enable_namespace(int argc, const char **argv, void *ctx)
+int cmd_enable_namespace(int argc, const char **argv, struct ndctl_ctx *ctx)
 {
 	char *xable_usage = "ndctl enable-namespace <namespace> [<options>]";
 	const char *namespace = parse_namespace_options(argc, argv,
@@ -1171,7 +1171,7 @@ int cmd_enable_namespace(int argc, const char **argv, void *ctx)
 	return rc;
 }
 
-int cmd_create_namespace(int argc, const char **argv, void *ctx)
+int cmd_create_namespace(int argc, const char **argv, struct ndctl_ctx *ctx)
 {
 	char *xable_usage = "ndctl create-namespace [<options>]";
 	const char *namespace = parse_namespace_options(argc, argv,
@@ -1200,7 +1200,7 @@ int cmd_create_namespace(int argc, const char **argv, void *ctx)
 	return rc;
 }
 
-int cmd_destroy_namespace(int argc , const char **argv, void *ctx)
+int cmd_destroy_namespace(int argc , const char **argv, struct ndctl_ctx *ctx)
 {
 	char *xable_usage = "ndctl destroy-namespace <namespace> [<options>]";
 	const char *namespace = parse_namespace_options(argc, argv,
@@ -1216,7 +1216,7 @@ int cmd_destroy_namespace(int argc , const char **argv, void *ctx)
 	return rc;
 }
 
-int cmd_check_namespace(int argc , const char **argv, void *ctx)
+int cmd_check_namespace(int argc , const char **argv, struct ndctl_ctx *ctx)
 {
 	char *xable_usage = "ndctl check-namespace <namespace> [<options>]";
 	const char *namespace = parse_namespace_options(argc, argv,
diff --git a/ndctl/ndctl.c b/ndctl/ndctl.c
index 9c930b92fe44..b01594e06739 100644
--- a/ndctl/ndctl.c
+++ b/ndctl/ndctl.c
@@ -35,13 +35,13 @@ static const char ndctl_more_info_string[] =
 	"See 'ndctl help COMMAND' for more information on a specific command.\n"
 	" ndctl --list-cmds to see all available commands";
 
-static int cmd_version(int argc, const char **argv, void *ctx)
+static int cmd_version(int argc, const char **argv, struct ndctl_ctx *ctx)
 {
 	printf("%s\n", VERSION);
 	return 0;
 }
 
-static int cmd_help(int argc, const char **argv, void *ctx)
+static int cmd_help(int argc, const char **argv, struct ndctl_ctx *ctx)
 {
 	const char * const builtin_help_subcommands[] = {
 		"enable-region", "disable-region", "zero-labels",
@@ -67,35 +67,35 @@ static int cmd_help(int argc, const char **argv, void *ctx)
 }
 
 static struct cmd_struct commands[] = {
-	{ "version", cmd_version },
-	{ "create-nfit", cmd_create_nfit },
-	{ "enable-namespace", cmd_enable_namespace },
-	{ "disable-namespace", cmd_disable_namespace },
-	{ "create-namespace", cmd_create_namespace },
-	{ "destroy-namespace", cmd_destroy_namespace },
-	{ "check-namespace", cmd_check_namespace },
-	{ "enable-region", cmd_enable_region },
-	{ "disable-region", cmd_disable_region },
-	{ "enable-dimm", cmd_enable_dimm },
-	{ "disable-dimm", cmd_disable_dimm },
-	{ "zero-labels", cmd_zero_labels },
-	{ "read-labels", cmd_read_labels },
-	{ "write-labels", cmd_write_labels },
-	{ "init-labels", cmd_init_labels },
-	{ "check-labels", cmd_check_labels },
-	{ "inject-error", cmd_inject_error },
-	{ "update-firmware", cmd_update_firmware },
-	{ "inject-smart", cmd_inject_smart },
-	{ "wait-scrub", cmd_wait_scrub },
-	{ "start-scrub", cmd_start_scrub },
-	{ "list", cmd_list },
-	{ "monitor", cmd_monitor},
-	{ "help", cmd_help },
+	{ "version", { cmd_version } },
+	{ "create-nfit", { cmd_create_nfit } },
+	{ "enable-namespace", { cmd_enable_namespace } },
+	{ "disable-namespace", { cmd_disable_namespace } },
+	{ "create-namespace", { cmd_create_namespace } },
+	{ "destroy-namespace", { cmd_destroy_namespace } },
+	{ "check-namespace", { cmd_check_namespace } },
+	{ "enable-region", { cmd_enable_region } },
+	{ "disable-region", { cmd_disable_region } },
+	{ "enable-dimm", { cmd_enable_dimm } },
+	{ "disable-dimm", { cmd_disable_dimm } },
+	{ "zero-labels", { cmd_zero_labels } },
+	{ "read-labels", { cmd_read_labels } },
+	{ "write-labels", { cmd_write_labels } },
+	{ "init-labels", { cmd_init_labels } },
+	{ "check-labels", { cmd_check_labels } },
+	{ "inject-error", { cmd_inject_error } },
+	{ "update-firmware", { cmd_update_firmware } },
+	{ "inject-smart", { cmd_inject_smart } },
+	{ "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 },
+	{ "test", { cmd_test } },
 	#endif
 	#ifdef ENABLE_DESTRUCTIVE
-	{ "bat", cmd_bat },
+	{ "bat", { cmd_bat } },
 	#endif
 };
 
@@ -124,7 +124,7 @@ int main(int argc, const char **argv)
 	if (rc)
 		goto out;
 	main_handle_internal_command(argc, argv, ctx, commands,
-			ARRAY_SIZE(commands));
+			ARRAY_SIZE(commands), PROG_NDCTL);
 	ndctl_unref(ctx);
 	fprintf(stderr, "Unknown command: '%s'\n", argv[0]);
 out:
diff --git a/ndctl/region.c b/ndctl/region.c
index 9fc90808e338..993b3f87eb40 100644
--- a/ndctl/region.c
+++ b/ndctl/region.c
@@ -124,7 +124,7 @@ static int do_xable_region(const char *region_arg, enum device_action mode,
 	return rc;
 }
 
-int cmd_disable_region(int argc, const char **argv, void *ctx)
+int cmd_disable_region(int argc, const char **argv, struct ndctl_ctx *ctx)
 {
 	char *xable_usage = "ndctl disable-region <region> [<options>]";
 	const char *region = parse_region_options(argc, argv, xable_usage);
@@ -144,7 +144,7 @@ int cmd_disable_region(int argc, const char **argv, void *ctx)
 	}
 }
 
-int cmd_enable_region(int argc, const char **argv, void *ctx)
+int cmd_enable_region(int argc, const char **argv, struct ndctl_ctx *ctx)
 {
 	char *xable_usage = "ndctl enable-region <region> [<options>]";
 	const char *region = parse_region_options(argc, argv, xable_usage);
diff --git a/ndctl/test.c b/ndctl/test.c
index 25ec61cdec1e..b78776311125 100644
--- a/ndctl/test.c
+++ b/ndctl/test.c
@@ -26,7 +26,7 @@ static char *result(int rc)
 		return "PASS";
 }
 
-int cmd_test(int argc, const char **argv, void *ctx)
+int cmd_test(int argc, const char **argv, struct ndctl_ctx *ctx)
 {
 	struct ndctl_test *test;
 	int loglevel = LOG_DEBUG, i, rc;
diff --git a/util/main.c b/util/main.c
index a7992589cfcc..4f925f84966a 100644
--- a/util/main.c
+++ b/util/main.c
@@ -82,12 +82,15 @@ int main_handle_options(const char ***argv, int *argc, const char *usage_msg,
 }
 
 static int run_builtin(struct cmd_struct *p, int argc, const char **argv,
-		void *ctx)
+		void *ctx, enum program prog)
 {
 	int status;
 	struct stat st;
 
-	status = p->fn(argc, argv, ctx);
+	if (prog == PROG_NDCTL)
+		status = p->n_fn(argc, argv, ctx);
+	else
+		status = p->d_fn(argc, argv, ctx);
 
 	if (status)
 		return status & 0xff;
@@ -119,7 +122,7 @@ out:
 }
 
 void main_handle_internal_command(int argc, const char **argv, void *ctx,
-		struct cmd_struct *cmds, int num_cmds)
+		struct cmd_struct *cmds, int num_cmds, enum program prog)
 {
 	const char *cmd = argv[0];
 	int i;
@@ -134,6 +137,6 @@ void main_handle_internal_command(int argc, const char **argv, void *ctx,
 		struct cmd_struct *p = cmds+i;
 		if (strcmp(p->cmd, cmd))
 			continue;
-		exit(run_builtin(p, argc, argv, ctx));
+		exit(run_builtin(p, argc, argv, ctx, prog));
 	}
 }
diff --git a/util/main.h b/util/main.h
index d61d10c79630..35fb33e63049 100644
--- a/util/main.h
+++ b/util/main.h
@@ -17,15 +17,26 @@
 #ifndef __MAIN_H__
 #define __MAIN_H__
 
+enum program {
+	PROG_NDCTL,
+	PROG_DAXCTL,
+};
+
+struct ndctl_ctx;
+struct daxctl_ctx;
+
 struct cmd_struct {
 	const char *cmd;
-	int (*fn)(int, const char **, void *ctx);
+	union {
+		int (*n_fn)(int, const char **, struct ndctl_ctx *ctx);
+		int (*d_fn)(int, const char **, struct daxctl_ctx *ctx);
+	};
 };
 
 int main_handle_options(const char ***argv, int *argc, const char *usage_msg,
 		struct cmd_struct *cmds, int num_cmds);
 void main_handle_internal_command(int argc, const char **argv, void *ctx,
-		struct cmd_struct *cmds, int num_cmds);
+		struct cmd_struct *cmds, int num_cmds, enum program prog);
 int help_show_man_page(const char *cmd, const char *util_name,
 		const char *viewer);
 #endif /* __MAIN_H__ */

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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [ndctl PATCH 3/7] ndctl/monitor: Drop 'struct ndctl_ctx *' casts
  2018-12-28  3:29 [ndctl PATCH 0/7] ndctl/monitor: Cleanups and fixes Dan Williams
  2018-12-28  3:30 ` [ndctl PATCH 1/7] ndctl, daxctl: Split builtin.h per-command Dan Williams
  2018-12-28  3:30 ` [ndctl PATCH 2/7] ndctl, daxctl: Add type-safety to command harness Dan Williams
@ 2018-12-28  3:30 ` Dan Williams
  2018-12-28  3:30 ` [ndctl PATCH 4/7] ndctl/monitor: Unify definition of default monitor configfile path Dan Williams
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Dan Williams @ 2018-12-28  3:30 UTC (permalink / raw)
  To: linux-nvdimm

Now that the command harness is type-safe with respect to the @ctx
argument. Drop the casts in cmd_monitor().

Cc: QI Fuli <qi.fuli@jp.fujitsu.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 ndctl/monitor.c |   22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/ndctl/monitor.c b/ndctl/monitor.c
index 47caae55d56d..08219f99b12b 100644
--- a/ndctl/monitor.c
+++ b/ndctl/monitor.c
@@ -623,14 +623,14 @@ int cmd_monitor(int argc, const char **argv, struct ndctl_ctx *ctx)
 		usage_with_options(u, options);
 
 	/* default to log_standard */
-	ndctl_set_log_fn((struct ndctl_ctx *)ctx, log_standard);
+	ndctl_set_log_fn(ctx, log_standard);
 
 	if (monitor.verbose)
-		ndctl_set_log_priority((struct ndctl_ctx *)ctx, LOG_DEBUG);
+		ndctl_set_log_priority(ctx, LOG_DEBUG);
 	else
-		ndctl_set_log_priority((struct ndctl_ctx *)ctx, LOG_INFO);
+		ndctl_set_log_priority(ctx, LOG_INFO);
 
-	rc = read_config_file((struct ndctl_ctx *)ctx, &monitor, &param);
+	rc = read_config_file(ctx, &monitor, &param);
 	if (rc)
 		goto out;
 
@@ -638,7 +638,7 @@ int cmd_monitor(int argc, const char **argv, struct ndctl_ctx *ctx)
 		if (strncmp(monitor.log, "./", 2) != 0)
 			fix_filename(prefix, (const char **)&monitor.log);
 		if (strncmp(monitor.log, "./syslog", 8) == 0)
-			ndctl_set_log_fn((struct ndctl_ctx *)ctx, log_syslog);
+			ndctl_set_log_fn(ctx, log_syslog);
 		else if (strncmp(monitor.log, "./standard", 10) == 0)
 			; /*default, already set */
 		else {
@@ -649,21 +649,21 @@ int cmd_monitor(int argc, const char **argv, struct ndctl_ctx *ctx)
 				goto out;
 			}
 			fclose(f);
-			ndctl_set_log_fn((struct ndctl_ctx *)ctx, log_file);
+			ndctl_set_log_fn(ctx, log_file);
 		}
 	}
 
 	if (monitor.daemon) {
 		if (!monitor.log || strncmp(monitor.log, "./", 2) == 0)
-			ndctl_set_log_fn((struct ndctl_ctx *)ctx, log_syslog);
+			ndctl_set_log_fn(ctx, log_syslog);
 		if (daemon(0, 0) != 0) {
-			err((struct ndctl_ctx *)ctx, "daemon start failed\n");
+			err(ctx, "daemon start failed\n");
 			goto out;
 		}
-		info((struct ndctl_ctx *)ctx, "ndctl monitor daemon started\n");
+		info(ctx, "ndctl monitor daemon started\n");
 	}
 
-	if (parse_monitor_event(&monitor, (struct ndctl_ctx *)ctx))
+	if (parse_monitor_event(&monitor, ctx))
 		goto out;
 
 	fctx.filter_bus = filter_bus;
@@ -681,7 +681,7 @@ int cmd_monitor(int argc, const char **argv, struct ndctl_ctx *ctx)
 		goto out;
 
 	if (!mfa.num_dimm) {
-		dbg((struct ndctl_ctx *)ctx, "no dimms to monitor\n");
+		dbg(ctx, "no dimms to monitor\n");
 		if (!monitor.daemon)
 			rc = -ENXIO;
 		goto out;

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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [ndctl PATCH 4/7] ndctl/monitor: Unify definition of default monitor configfile path
  2018-12-28  3:29 [ndctl PATCH 0/7] ndctl/monitor: Cleanups and fixes Dan Williams
                   ` (2 preceding siblings ...)
  2018-12-28  3:30 ` [ndctl PATCH 3/7] ndctl/monitor: Drop 'struct ndctl_ctx *' casts Dan Williams
@ 2018-12-28  3:30 ` Dan Williams
  2018-12-28  3:30 ` [ndctl PATCH 5/7] ndctl/monitor: Fix / cleanup log_file() Dan Williams
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Dan Williams @ 2018-12-28  3:30 UTC (permalink / raw)
  To: linux-nvdimm

The definition of the monitor configuration file can get out of sync
with current duplication between configure.ac and ndctl/Makefile.am.
Instead, define all the variables in configure.ac and just reference
them in ndctl/Makefile.am.

Cc: QI Fuli <qi.fuli@jp.fujitsu.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 configure.ac      |    8 +++++++-
 ndctl/Makefile.am |    6 ++----
 ndctl/monitor.c   |    2 +-
 3 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/configure.ac b/configure.ac
index de5b84cec670..aa07ec7bc870 100644
--- a/configure.ac
+++ b/configure.ac
@@ -154,8 +154,14 @@ fi
 AC_SUBST([systemd_unitdir])
 AM_CONDITIONAL([ENABLE_SYSTEMD_UNITS], [test "x$with_systemd" = "xyes"])
 
+ndctl_monitorconfdir=${sysconfdir}/ndctl
+ndctl_monitorconf=monitor.conf
+AC_SUBST([ndctl_monitorconfdir])
+AC_SUBST([ndctl_monitorconf])
+AC_DEFINE_UNQUOTED(NDCTL_CONF_FILE, ["$ndctl_monitorconfdir/$ndctl_monitorconf"],
+	[default ndctl monitor conf path])
+
 my_CFLAGS="\
--D DEF_CONF_FILE='\"${sysconfdir}/ndctl/monitor.conf\"' \
 -Wall \
 -Wchar-subscripts \
 -Wformat-security \
diff --git a/ndctl/Makefile.am b/ndctl/Makefile.am
index 8a5e5f87e6c5..ff01e0688afd 100644
--- a/ndctl/Makefile.am
+++ b/ndctl/Makefile.am
@@ -43,10 +43,8 @@ ndctl_SOURCES += ../test/libndctl.c \
 		 test.c
 endif
 
-monitor_config_file = monitor.conf
-monitor_configdir = $(sysconfdir)/ndctl/
-monitor_config_DATA = $(monitor_config_file)
-EXTRA_DIST += $(monitor_config_file)
+monitor_configdir = $(ndctl_monitorconfdir)
+monitor_config_DATA = $(ndctl_monitorconf)
 
 if ENABLE_SYSTEMD_UNITS
 systemd_unit_DATA = ndctl-monitor.service
diff --git a/ndctl/monitor.c b/ndctl/monitor.c
index 08219f99b12b..cef70d06beae 100644
--- a/ndctl/monitor.c
+++ b/ndctl/monitor.c
@@ -503,7 +503,7 @@ static int read_config_file(struct ndctl_ctx *ctx, struct monitor *_monitor,
 	if (_monitor->config_file)
 		config_file = strdup(_monitor->config_file);
 	else
-		config_file = strdup(DEF_CONF_FILE);
+		config_file = strdup(NDCTL_CONF_FILE);
 	if (!config_file) {
 		fail("strdup default config file failed\n");
 		rc = -ENOMEM;

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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [ndctl PATCH 5/7] ndctl/monitor: Fix / cleanup log_file()
  2018-12-28  3:29 [ndctl PATCH 0/7] ndctl/monitor: Cleanups and fixes Dan Williams
                   ` (3 preceding siblings ...)
  2018-12-28  3:30 ` [ndctl PATCH 4/7] ndctl/monitor: Unify definition of default monitor configfile path Dan Williams
@ 2018-12-28  3:30 ` Dan Williams
  2018-12-28  3:30 ` [ndctl PATCH 6/7] ndctl/monitor: Drop vasprintf usage Dan Williams
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Dan Williams @ 2018-12-28  3:30 UTC (permalink / raw)
  To: linux-nvdimm

The log_file() helper opens the log file for each message. Avoid the
scenario where the fopen() fails and just open it at init time.

This usage is also broken because if the vasprintf() fails it will try
to print via log_file again via fail(), and potentially lead to infinite
recursion.

Cc: Qi Fuli <qi.fuli@jp.fujitsu.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 ndctl/monitor.c |   23 ++++++-----------------
 1 file changed, 6 insertions(+), 17 deletions(-)

diff --git a/ndctl/monitor.c b/ndctl/monitor.c
index cef70d06beae..8a16c9664e0a 100644
--- a/ndctl/monitor.c
+++ b/ndctl/monitor.c
@@ -20,6 +20,7 @@ static struct monitor {
 	const char *log;
 	const char *config_file;
 	const char *dimm_event;
+	FILE *log_file;
 	bool daemon;
 	bool human;
 	bool verbose;
@@ -82,7 +83,7 @@ static void log_standard(struct ndctl_ctx *ctx, int priority, const char *file,
 static void log_file(struct ndctl_ctx *ctx, int priority, const char *file,
 		int line, const char *fn, const char *format, va_list args)
 {
-	FILE *f;
+	FILE *f = monitor.log_file;
 	char *buf;
 	struct timespec ts;
 	char timestamp[32];
@@ -92,16 +93,6 @@ static void log_file(struct ndctl_ctx *ctx, int priority, const char *file,
 		return;
 	}
 
-	f = fopen(monitor.log, "a+");
-	if (!f) {
-		ndctl_set_log_fn(ctx, log_syslog);
-		err(ctx, "open logfile %s failed, forward messages to syslog\n",
-				monitor.log);
-		did_fail = 1;
-		notice(ctx, "%s\n", buf);
-		goto end;
-	}
-
 	if (priority != LOG_NOTICE) {
 		clock_gettime(CLOCK_REALTIME, &ts);
 		sprintf(timestamp, "%10ld.%09ld", ts.tv_sec, ts.tv_nsec);
@@ -110,8 +101,6 @@ static void log_file(struct ndctl_ctx *ctx, int priority, const char *file,
 		fprintf(f, "%s", buf);
 
 	fflush(f);
-	fclose(f);
-end:
 	free(buf);
 	return;
 }
@@ -613,7 +602,6 @@ int cmd_monitor(int argc, const char **argv, struct ndctl_ctx *ctx)
 	struct util_filter_ctx fctx = { 0 };
 	struct monitor_filter_arg mfa = { 0 };
 	int i, rc;
-	FILE *f;
 
 	argc = parse_options_prefix(argc, argv, prefix, options, u, 0);
 	for (i = 0; i < argc; i++) {
@@ -642,13 +630,12 @@ int cmd_monitor(int argc, const char **argv, struct ndctl_ctx *ctx)
 		else if (strncmp(monitor.log, "./standard", 10) == 0)
 			; /*default, already set */
 		else {
-			f = fopen(monitor.log, "a+");
-			if (!f) {
+			monitor.log_file = fopen(monitor.log, "a+");
+			if (!monitor.log_file) {
 				error("open %s failed\n", monitor.log);
 				rc = -errno;
 				goto out;
 			}
-			fclose(f);
 			ndctl_set_log_fn(ctx, log_file);
 		}
 	}
@@ -689,5 +676,7 @@ int cmd_monitor(int argc, const char **argv, struct ndctl_ctx *ctx)
 
 	rc = monitor_event(ctx, &mfa);
 out:
+	if (monitor.log_file)
+		fclose(monitor.log_file);
 	return rc;
 }

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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [ndctl PATCH 6/7] ndctl/monitor: Drop vasprintf usage
  2018-12-28  3:29 [ndctl PATCH 0/7] ndctl/monitor: Cleanups and fixes Dan Williams
                   ` (4 preceding siblings ...)
  2018-12-28  3:30 ` [ndctl PATCH 5/7] ndctl/monitor: Fix / cleanup log_file() Dan Williams
@ 2018-12-28  3:30 ` Dan Williams
  2018-12-28  3:30 ` [ndctl PATCH 7/7] ndctl/monitor: Kill usage of ndctl/lib/private.h Dan Williams
  2019-01-03  0:37 ` [ndctl PATCH 0/7] ndctl/monitor: Cleanups and fixes Verma, Vishal L
  7 siblings, 0 replies; 9+ messages in thread
From: Dan Williams @ 2018-12-28  3:30 UTC (permalink / raw)
  To: linux-nvdimm

Clean up some unnecessary buffer allocations for printing, when
vfprintf() and vsyslog() can be used directly.

Cc: QI Fuli <qi.fuli@jp.fujitsu.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 ndctl/monitor.c |   43 ++++++++-----------------------------------
 1 file changed, 8 insertions(+), 35 deletions(-)

diff --git a/ndctl/monitor.c b/ndctl/monitor.c
index 8a16c9664e0a..e38a570fe960 100644
--- a/ndctl/monitor.c
+++ b/ndctl/monitor.c
@@ -49,60 +49,33 @@ do { \
 static void log_syslog(struct ndctl_ctx *ctx, int priority, const char *file,
 		int line, const char *fn, const char *format, va_list args)
 {
-	char *buf;
-
-	if (vasprintf(&buf, format, args) < 0) {
-		fail("vasprintf error\n");
-		return;
-	}
-	syslog(priority, "%s", buf);
-
-	free(buf);
-	return;
+	vsyslog(priority, format, args);
 }
 
 static void log_standard(struct ndctl_ctx *ctx, int priority, const char *file,
 		int line, const char *fn, const char *format, va_list args)
 {
-	char *buf;
-
-	if (vasprintf(&buf, format, args) < 0) {
-		fail("vasprintf error\n");
-		return;
-	}
-
 	if (priority == 6)
-		fprintf(stdout, "%s", buf);
+		vfprintf(stdout, format, args);
 	else
-		fprintf(stderr, "%s", buf);
-
-	free(buf);
-	return;
+		vfprintf(stderr, format, args);
 }
 
 static void log_file(struct ndctl_ctx *ctx, int priority, const char *file,
 		int line, const char *fn, const char *format, va_list args)
 {
 	FILE *f = monitor.log_file;
-	char *buf;
-	struct timespec ts;
-	char timestamp[32];
-
-	if (vasprintf(&buf, format, args) < 0) {
-		fail("vasprintf error\n");
-		return;
-	}
 
 	if (priority != LOG_NOTICE) {
+		struct timespec ts;
+
 		clock_gettime(CLOCK_REALTIME, &ts);
-		sprintf(timestamp, "%10ld.%09ld", ts.tv_sec, ts.tv_nsec);
-		fprintf(f, "[%s] [%d] %s", timestamp, getpid(), buf);
+		fprintf(f, "[%10ld.%09ld] [%d] ", ts.tv_sec, ts.tv_nsec, getpid());
+		vfprintf(f, format, args);
 	} else
-		fprintf(f, "%s", buf);
+		vfprintf(f, format, args);
 
 	fflush(f);
-	free(buf);
-	return;
 }
 
 static struct json_object *dimm_event_to_json(struct monitor_dimm *mdimm)

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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [ndctl PATCH 7/7] ndctl/monitor: Kill usage of ndctl/lib/private.h
  2018-12-28  3:29 [ndctl PATCH 0/7] ndctl/monitor: Cleanups and fixes Dan Williams
                   ` (5 preceding siblings ...)
  2018-12-28  3:30 ` [ndctl PATCH 6/7] ndctl/monitor: Drop vasprintf usage Dan Williams
@ 2018-12-28  3:30 ` Dan Williams
  2019-01-03  0:37 ` [ndctl PATCH 0/7] ndctl/monitor: Cleanups and fixes Verma, Vishal L
  7 siblings, 0 replies; 9+ messages in thread
From: Dan Williams @ 2018-12-28  3:30 UTC (permalink / raw)
  To: linux-nvdimm

The 'private.h' header is only to be consumed internally by libndctl.
The monitor should be using its own log context, not reusing the
library context which might be injecting it's own messages into the log
stream.

Cc: Qi Fuli <qi.fuli@jp.fujitsu.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 ndctl/inject-error.c |    1 -
 ndctl/inject-smart.c |    1 -
 ndctl/monitor.c      |   89 ++++++++++++++++++++++++++------------------------
 3 files changed, 47 insertions(+), 44 deletions(-)

diff --git a/ndctl/inject-error.c b/ndctl/inject-error.c
index fef6f75716be..fe599efc31a6 100644
--- a/ndctl/inject-error.c
+++ b/ndctl/inject-error.c
@@ -33,7 +33,6 @@
 #include <ccan/array_size/array_size.h>
 #include <ccan/short_types/short_types.h>
 
-#include "private.h"
 #include <builtin.h>
 #include <test.h>
 
diff --git a/ndctl/inject-smart.c b/ndctl/inject-smart.c
index 7a44a684d630..eaa137aaede1 100644
--- a/ndctl/inject-smart.c
+++ b/ndctl/inject-smart.c
@@ -24,7 +24,6 @@
 #include <ccan/array_size/array_size.h>
 #include <ccan/short_types/short_types.h>
 
-#include "private.h"
 #include <builtin.h>
 #include <test.h>
 
diff --git a/ndctl/monitor.c b/ndctl/monitor.c
index e38a570fe960..233f2bbd9b55 100644
--- a/ndctl/monitor.c
+++ b/ndctl/monitor.c
@@ -5,17 +5,25 @@
 #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/ndctl.h>
 #include <ndctl/libndctl.h>
 #include <sys/epoll.h>
 #define BUF_SIZE 2048
 
+/* reuse the core log helpers for the monitor logger */
+#ifndef ENABLE_LOGGING
+#define ENABLE_LOGGING
+#endif
+#ifndef ENABLE_DEBUG
+#define ENABLE_DEBUG
+#endif
+#include <util/log.h>
+
 static struct monitor {
 	const char *log;
 	const char *config_file;
@@ -25,6 +33,7 @@ static struct monitor {
 	bool human;
 	bool verbose;
 	unsigned int event_flags;
+	struct log_ctx ctx;
 } monitor;
 
 struct monitor_dimm {
@@ -42,17 +51,17 @@ static int did_fail;
 #define fail(fmt, ...) \
 do { \
 	did_fail = 1; \
-	dbg(ctx, "ndctl-%s:%s:%d: " fmt, \
+	dbg(&monitor, "ndctl-%s:%s:%d: " fmt, \
 			VERSION, __func__, __LINE__, ##__VA_ARGS__); \
 } while (0)
 
-static void log_syslog(struct ndctl_ctx *ctx, int priority, const char *file,
+static void log_syslog(struct log_ctx *ctx, int priority, const char *file,
 		int line, const char *fn, const char *format, va_list args)
 {
 	vsyslog(priority, format, args);
 }
 
-static void log_standard(struct ndctl_ctx *ctx, int priority, const char *file,
+static void log_standard(struct log_ctx *ctx, int priority, const char *file,
 		int line, const char *fn, const char *format, va_list args)
 {
 	if (priority == 6)
@@ -61,7 +70,7 @@ static void log_standard(struct ndctl_ctx *ctx, int priority, const char *file,
 		vfprintf(stderr, format, args);
 }
 
-static void log_file(struct ndctl_ctx *ctx, int priority, const char *file,
+static void log_file(struct log_ctx *ctx, int priority, const char *file,
 		int line, const char *fn, const char *format, va_list args)
 {
 	FILE *f = monitor.log_file;
@@ -83,7 +92,6 @@ static struct json_object *dimm_event_to_json(struct monitor_dimm *mdimm)
 	struct json_object *jevent, *jobj;
 	bool spares_flag, media_temp_flag, ctrl_temp_flag,
 			health_state_flag, unclean_shutdown_flag;
-	struct ndctl_ctx *ctx = ndctl_dimm_get_ctx(mdimm->dimm);
 
 	jevent = json_object_new_object();
 	if (!jevent) {
@@ -144,7 +152,6 @@ static int notify_dimm_event(struct monitor_dimm *mdimm)
 	struct json_object *jmsg, *jdimm, *jobj;
 	struct timespec ts;
 	char timestamp[32];
-	struct ndctl_ctx *ctx = ndctl_dimm_get_ctx(mdimm->dimm);
 
 	jmsg = json_object_new_object();
 	if (!jmsg) {
@@ -175,10 +182,10 @@ static int notify_dimm_event(struct monitor_dimm *mdimm)
 		json_object_object_add(jdimm, "health", jobj);
 
 	if (monitor.human)
-		notice(ctx, "%s\n", json_object_to_json_string_ext(jmsg,
+		notice(&monitor, "%s\n", json_object_to_json_string_ext(jmsg,
 						JSON_C_TO_STRING_PRETTY));
 	else
-		notice(ctx, "%s\n", json_object_to_json_string_ext(jmsg,
+		notice(&monitor, "%s\n", json_object_to_json_string_ext(jmsg,
 						JSON_C_TO_STRING_PLAIN));
 
 	free(jobj);
@@ -213,21 +220,20 @@ static int enable_dimm_supported_threshold_alarms(struct ndctl_dimm *dimm)
 	int rc = -EOPNOTSUPP;
 	struct ndctl_cmd *st_cmd = NULL, *sst_cmd = NULL;
 	const char *name = ndctl_dimm_get_devname(dimm);
-	struct ndctl_ctx *ctx = ndctl_dimm_get_ctx(dimm);
 
 	st_cmd = ndctl_dimm_cmd_new_smart_threshold(dimm);
 	if (!st_cmd) {
-		err(ctx, "%s: no smart threshold command support\n", name);
+		err(&monitor, "%s: no smart threshold command support\n", name);
 		goto out;
 	}
 	if (ndctl_cmd_submit(st_cmd)) {
-		err(ctx, "%s: smart threshold command failed\n", name);
+		err(&monitor, "%s: smart threshold command failed\n", name);
 		goto out;
 	}
 
 	sst_cmd = ndctl_dimm_cmd_new_smart_set_threshold(st_cmd);
 	if (!sst_cmd) {
-		err(ctx, "%s: no smart set threshold command support\n", name);
+		err(&monitor, "%s: no smart set threshold command support\n", name);
 		goto out;
 	}
 
@@ -242,7 +248,7 @@ static int enable_dimm_supported_threshold_alarms(struct ndctl_dimm *dimm)
 
 	rc = ndctl_cmd_submit(sst_cmd);
 	if (rc) {
-		err(ctx, "%s: smart set threshold command failed\n", name);
+		err(&monitor, "%s: smart set threshold command failed\n", name);
 		goto out;
 	}
 
@@ -262,31 +268,30 @@ static void filter_dimm(struct ndctl_dimm *dimm, struct util_filter_ctx *fctx)
 {
 	struct monitor_dimm *mdimm;
 	struct monitor_filter_arg *mfa = fctx->monitor;
-	struct ndctl_ctx *ctx = ndctl_dimm_get_ctx(dimm);
 	const char *name = ndctl_dimm_get_devname(dimm);
 
 	if (!ndctl_dimm_is_cmd_supported(dimm, ND_CMD_SMART)) {
-		err(ctx, "%s: no smart support\n", name);
+		err(&monitor, "%s: no smart support\n", name);
 		return;
 	}
 	if (!ndctl_dimm_is_cmd_supported(dimm, ND_CMD_SMART_THRESHOLD)) {
-		err(ctx, "%s: no smart threshold support\n", name);
+		err(&monitor, "%s: no smart threshold support\n", name);
 		return;
 	}
 
 	if (!ndctl_dimm_is_flag_supported(dimm, ND_SMART_ALARM_VALID)) {
-		err(ctx, "%s: smart alarm invalid\n", name);
+		err(&monitor, "%s: smart alarm invalid\n", name);
 		return;
 	}
 
 	if (enable_dimm_supported_threshold_alarms(dimm)) {
-		err(ctx, "%s: enable supported threshold alarms failed\n", name);
+		err(&monitor, "%s: enable supported threshold alarms failed\n", name);
 		return;
 	}
 
 	mdimm = calloc(1, sizeof(struct monitor_dimm));
 	if (!mdimm) {
-		err(ctx, "%s: calloc for monitor dimm failed\n", name);
+		err(&monitor, "%s: calloc for monitor dimm failed\n", name);
 		return;
 	}
 
@@ -298,7 +303,7 @@ static void filter_dimm(struct ndctl_dimm *dimm, struct util_filter_ctx *fctx)
 	if (mdimm->event_flags
 			&& util_dimm_event_filter(mdimm, monitor.event_flags)) {
 		if (notify_dimm_event(mdimm)) {
-			err(ctx, "%s: notify dimm event failed\n", name);
+			err(&monitor, "%s: notify dimm event failed\n", name);
 			free(mdimm);
 			return;
 		}
@@ -326,12 +331,12 @@ static int monitor_event(struct ndctl_ctx *ctx,
 
 	events = calloc(mfa->num_dimm, sizeof(struct epoll_event));
 	if (!events) {
-		err(ctx, "malloc for events error\n");
+		err(&monitor, "malloc for events error\n");
 		return -ENOMEM;
 	}
 	epollfd = epoll_create1(0);
 	if (epollfd == -1) {
-		err(ctx, "epoll_create1 error\n");
+		err(&monitor, "epoll_create1 error\n");
 		rc = -errno;
 		goto out;
 	}
@@ -339,14 +344,14 @@ static int monitor_event(struct ndctl_ctx *ctx,
 		memset(&ev, 0, sizeof(ev));
 		rc = pread(mdimm->health_eventfd, &buf, sizeof(buf), 0);
 		if (rc < 0) {
-			err(ctx, "pread error\n");
+			err(&monitor, "pread error\n");
 			rc = -errno;
 			goto out;
 		}
 		ev.data.ptr = mdimm;
 		if (epoll_ctl(epollfd, EPOLL_CTL_ADD,
 				mdimm->health_eventfd, &ev) != 0) {
-			err(ctx, "epoll_ctl error\n");
+			err(&monitor, "epoll_ctl error\n");
 			rc = -errno;
 			goto out;
 		}
@@ -356,7 +361,7 @@ static int monitor_event(struct ndctl_ctx *ctx,
 		did_fail = 0;
 		nfds = epoll_wait(epollfd, events, mfa->num_dimm, -1);
 		if (nfds <= 0) {
-			err(ctx, "epoll_wait error\n");
+			err(&monitor, "epoll_wait error\n");
 			rc = -errno;
 			goto out;
 		}
@@ -365,7 +370,7 @@ static int monitor_event(struct ndctl_ctx *ctx,
 			if (util_dimm_event_filter(mdimm, monitor.event_flags)) {
 				rc = notify_dimm_event(mdimm);
 				if (rc) {
-					err(ctx, "%s: notify dimm event failed\n",
+					err(&monitor, "%s: notify dimm event failed\n",
 						ndctl_dimm_get_devname(mdimm->dimm));
 					did_fail = 1;
 					goto out;
@@ -373,7 +378,7 @@ static int monitor_event(struct ndctl_ctx *ctx,
 			}
 			rc = pread(mdimm->health_eventfd, &buf, sizeof(buf), 0);
 			if (rc < 0) {
-				err(ctx, "pread error\n");
+				err(&monitor, "pread error\n");
 				rc = -errno;
 				goto out;
 			}
@@ -427,7 +432,7 @@ static int parse_monitor_event(struct monitor *_monitor, struct ndctl_ctx *ctx)
 		else if (strcmp(event, "dimm-unclean-shutdown") == 0)
 			_monitor->event_flags |= ND_EVENT_UNCLEAN_SHUTDOWN;
 		else {
-			err(ctx, "no dimm-event named %s\n", event);
+			err(&monitor, "no dimm-event named %s\n", event);
 			rc = -EINVAL;
 			goto out;
 		}
@@ -482,7 +487,7 @@ static int read_config_file(struct ndctl_ctx *ctx, struct monitor *_monitor,
 
 	f = fopen(config_file, "r");
 	if (!f) {
-		err(ctx, "config-file: %s cannot be opened\n", config_file);
+		err(&monitor, "config-file: %s cannot be opened\n", config_file);
 		rc = -errno;
 		goto out;
 	}
@@ -583,13 +588,13 @@ int cmd_monitor(int argc, const char **argv, struct ndctl_ctx *ctx)
 	if (argc)
 		usage_with_options(u, options);
 
-	/* default to log_standard */
-	ndctl_set_log_fn(ctx, log_standard);
+	log_init(&monitor.ctx, "ndctl/monitor", "NDCTL_MONITOR_LOG");
+	monitor.ctx.log_fn = log_standard;
 
 	if (monitor.verbose)
-		ndctl_set_log_priority(ctx, LOG_DEBUG);
+		monitor.ctx.log_priority = LOG_DEBUG;
 	else
-		ndctl_set_log_priority(ctx, LOG_INFO);
+		monitor.ctx.log_priority = LOG_INFO;
 
 	rc = read_config_file(ctx, &monitor, &param);
 	if (rc)
@@ -599,9 +604,9 @@ int cmd_monitor(int argc, const char **argv, struct ndctl_ctx *ctx)
 		if (strncmp(monitor.log, "./", 2) != 0)
 			fix_filename(prefix, (const char **)&monitor.log);
 		if (strncmp(monitor.log, "./syslog", 8) == 0)
-			ndctl_set_log_fn(ctx, log_syslog);
+			monitor.ctx.log_fn = log_syslog;
 		else if (strncmp(monitor.log, "./standard", 10) == 0)
-			; /*default, already set */
+			monitor.ctx.log_fn = log_standard;
 		else {
 			monitor.log_file = fopen(monitor.log, "a+");
 			if (!monitor.log_file) {
@@ -609,18 +614,18 @@ int cmd_monitor(int argc, const char **argv, struct ndctl_ctx *ctx)
 				rc = -errno;
 				goto out;
 			}
-			ndctl_set_log_fn(ctx, log_file);
+			monitor.ctx.log_fn = log_file;
 		}
 	}
 
 	if (monitor.daemon) {
 		if (!monitor.log || strncmp(monitor.log, "./", 2) == 0)
-			ndctl_set_log_fn(ctx, log_syslog);
+			monitor.ctx.log_fn = log_syslog;
 		if (daemon(0, 0) != 0) {
-			err(ctx, "daemon start failed\n");
+			err(&monitor, "daemon start failed\n");
 			goto out;
 		}
-		info(ctx, "ndctl monitor daemon started\n");
+		info(&monitor, "ndctl monitor daemon started\n");
 	}
 
 	if (parse_monitor_event(&monitor, ctx))
@@ -641,7 +646,7 @@ int cmd_monitor(int argc, const char **argv, struct ndctl_ctx *ctx)
 		goto out;
 
 	if (!mfa.num_dimm) {
-		dbg(ctx, "no dimms to monitor\n");
+		dbg(&monitor, "no dimms to monitor\n");
 		if (!monitor.daemon)
 			rc = -ENXIO;
 		goto out;

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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [ndctl PATCH 0/7] ndctl/monitor: Cleanups and fixes
  2018-12-28  3:29 [ndctl PATCH 0/7] ndctl/monitor: Cleanups and fixes Dan Williams
                   ` (6 preceding siblings ...)
  2018-12-28  3:30 ` [ndctl PATCH 7/7] ndctl/monitor: Kill usage of ndctl/lib/private.h Dan Williams
@ 2019-01-03  0:37 ` Verma, Vishal L
  7 siblings, 0 replies; 9+ messages in thread
From: Verma, Vishal L @ 2019-01-03  0:37 UTC (permalink / raw)
  To: Williams, Dan J, linux-nvdimm


On Thu, 2018-12-27 at 19:29 -0800, Dan Williams wrote:
> Prompted by a need to add more commands to daxctl, and define a new
> configuration file for daxctl to install, I took a look at the ndctl
> monitor configuration file implementation and several fixes fell out.
> 
> An initial attempt to remove casts from the ndctl monitor uncovered
> other cleanup opportunities. The motivation for some of the casts in
> ndctl/monitor.c was due to the need to de-reference the 'ctx' parameter
> passed to the log routines. That issue can be mitigated by teaching the
> command harness to pass a typed version of the @ctx argument to the
> builtin-command routines. However, looking closer, the monitor should
> not be passing @ctx to the log routines, it should establish its own
> log-context. That lead to the discovery of a few more cleanup
> opportunities, like unnecessary usage of vaprintf().
> 
> More is possible. I am not comfortable with the fact that the log
> facility dynamically changes the output and the output target based on
> the priority. The monitor also has several occasions where it is
> dynamically allocating memory unnecessarily.
> 
> ---
> 
> Dan Williams (7):
>       ndctl, daxctl: Split builtin.h per-command
>       ndctl, daxctl: Add type-safety to command harness
>       ndctl/monitor: Drop 'struct ndctl_ctx *' casts
>       ndctl/monitor: Unify definition of default monitor configfile path
>       ndctl/monitor: Fix / cleanup log_file()
>       ndctl/monitor: Drop vasprintf usage
>       ndctl/monitor: Kill usage of ndctl/lib/private.h
> 

These look good, applied.

> 
>  builtin.h            |   51 ----------------
>  configure.ac         |    8 ++
>  daxctl/builtin.h     |    8 ++
>  daxctl/daxctl.c      |   16 ++---
>  daxctl/list.c        |    2 -
>  ndctl/Makefile.am    |    6 +-
>  ndctl/bat.c          |    2 -
>  ndctl/builtin.h      |   35 +++++++++++
>  ndctl/bus.c          |    4 +
>  ndctl/create-nfit.c  |    2 -
>  ndctl/dimm.c         |   18 +++---
>  ndctl/inject-error.c |    3 -
>  ndctl/inject-smart.c |    3 -
>  ndctl/list.c         |    2 -
>  ndctl/monitor.c      |  163 ++++++++++++++++++++------------------------------
>  ndctl/namespace.c    |   10 ++-
>  ndctl/ndctl.c        |   64 ++++++++++----------
>  ndctl/region.c       |    4 +
>  ndctl/test.c         |    2 -
>  util/main.c          |   13 ++--
>  util/main.h          |   20 ++++++
>  21 files changed, 207 insertions(+), 229 deletions(-)
>  delete mode 100644 builtin.h
>  create mode 100644 daxctl/builtin.h
>  create mode 100644 ndctl/builtin.h

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2019-01-03  0:37 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-28  3:29 [ndctl PATCH 0/7] ndctl/monitor: Cleanups and fixes Dan Williams
2018-12-28  3:30 ` [ndctl PATCH 1/7] ndctl, daxctl: Split builtin.h per-command Dan Williams
2018-12-28  3:30 ` [ndctl PATCH 2/7] ndctl, daxctl: Add type-safety to command harness Dan Williams
2018-12-28  3:30 ` [ndctl PATCH 3/7] ndctl/monitor: Drop 'struct ndctl_ctx *' casts Dan Williams
2018-12-28  3:30 ` [ndctl PATCH 4/7] ndctl/monitor: Unify definition of default monitor configfile path Dan Williams
2018-12-28  3:30 ` [ndctl PATCH 5/7] ndctl/monitor: Fix / cleanup log_file() Dan Williams
2018-12-28  3:30 ` [ndctl PATCH 6/7] ndctl/monitor: Drop vasprintf usage Dan Williams
2018-12-28  3:30 ` [ndctl PATCH 7/7] ndctl/monitor: Kill usage of ndctl/lib/private.h Dan Williams
2019-01-03  0:37 ` [ndctl PATCH 0/7] ndctl/monitor: Cleanups and fixes Verma, Vishal L

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.