[ndctl PATCH 0/7] Policy based reconfiguration for daxctl

[ndctl PATCH 0/7] Policy based reconfiguration for daxctl

[Vishal Verma]

These patches add policy (config file) support to daxctl. The
introductory user is daxctl-reconfigure-device. Sysadmins may wish to
use daxctl devices as system-ram, but it may be cumbersome to automate
the reconfiguration step for every device upon boot.

Introduce a new option for daxctl-reconfigure-device, --check-config.
This is at the heart of policy based reconfiguration, as it allows
daxctl to look up reconfiguration parameters for a given device from the
config system instead of the command line.

Some systemd and udev glue then automates this for every new dax device
that shows up, providing a way for the administrator to simply list all
the 'system-ram' UUIDs in a config file, and not have to worry about
anything else.

An example config file can be:

  # cat /etc/ndctl/daxctl.conf

  [auto-online unique_identifier_foo]
  uuid = 48d8e42c-a2f0-4312-9e70-a837faafe862
  mode = system-ram
  online = true
  movable = false

Any file under '/etc/ndctl/' can be used - all files with a '.conf' suffix
will be considered when looking for matches.

These patches depend on the initial config file support from Qi Fuli[1].
A branch containing these patches is available at [2].

[1]: https://lore.kernel.org/nvdimm/20210824095106.104808-1-qi.fuli@fujitsu.com/
[2]: https://github.com/pmem/ndctl/tree/vv/daxctl_config

Vishal Verma (7):
  ndctl: Update ndctl.spec.in for 'ndctl.conf'
  daxctl: Documentation updates for persistent reconfiguration
  util/parse-config: refactor filter_conf_files into util/
  daxctl: add basic config parsing support
  util/parse-configs: add a key/value search helper
  daxctl/device.c: add an option for getting params from a config file
  daxctl: add systemd service and udev rule for auto-onlining

 .../daxctl/daxctl-reconfigure-device.txt      |  67 +++++++++
 configure.ac                                  |   9 +-
 daxctl/lib/libdaxctl.c                        |  37 +++++
 ndctl/lib/libndctl.c                          |  19 +--
 daxctl/libdaxctl.h                            |   2 +
 util/parse-configs.h                          |  19 +++
 daxctl/daxctl.c                               |   1 +
 daxctl/device.c                               | 141 +++++++++++++++++-
 util/parse-configs.c                          |  67 +++++++++
 daxctl/90-daxctl-device.rules                 |   1 +
 daxctl/Makefile.am                            |  12 ++
 daxctl/daxdev-auto-reconfigure.sh             |   3 +
 daxctl/daxdev-reconfigure@.service            |   8 +
 daxctl/lib/Makefile.am                        |   6 +
 daxctl/lib/libdaxctl.sym                      |   2 +
 ndctl.spec.in                                 |   4 +
 ndctl/lib/Makefile.am                         |   2 +
 17 files changed, 381 insertions(+), 19 deletions(-)
 create mode 100644 daxctl/90-daxctl-device.rules
 create mode 100755 daxctl/daxdev-auto-reconfigure.sh
 create mode 100644 daxctl/daxdev-reconfigure@.service


base-commit: 5f1026ef3ad108f3f5aa889ef15edae92cb5de43
-- 
2.31.1

[ndctl PATCH 1/7] ndctl: Update ndctl.spec.in for 'ndctl.conf'

[Vishal Verma]

The new config system introduces and installs a sample config file
called ndctl.conf. Update the RPM spec to include this in the %files
section for ndctl.

Cc: QI Fuli <qi.fuli@fujitsu.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 ndctl.spec.in | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ndctl.spec.in b/ndctl.spec.in
index 0563b2d..07c36ec 100644
--- a/ndctl.spec.in
+++ b/ndctl.spec.in
@@ -118,6 +118,7 @@ make check
 %{_sysconfdir}/modprobe.d/nvdimm-security.conf
 
 %config(noreplace) %{_sysconfdir}/ndctl/monitor.conf
+%config(noreplace) %{_sysconfdir}/ndctl/ndctl.conf
 
 %files -n daxctl
 %defattr(-,root,root)
-- 
2.31.1

[ndctl PATCH 2/7] daxctl: Documentation updates for persistent reconfiguration

[Vishal Verma]

Add a man page update describing how daxctl-reconfigure-device(1) can
be used for persistent reconfiguration of a daxctl device using a
config file.

Cc: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 .../daxctl/daxctl-reconfigure-device.txt      | 67 +++++++++++++++++++
 1 file changed, 67 insertions(+)

diff --git a/Documentation/daxctl/daxctl-reconfigure-device.txt b/Documentation/daxctl/daxctl-reconfigure-device.txt
index f112b3c..1e2d380 100644
--- a/Documentation/daxctl/daxctl-reconfigure-device.txt
+++ b/Documentation/daxctl/daxctl-reconfigure-device.txt
@@ -162,6 +162,15 @@ include::region-option.txt[]
 	brought online automatically and immediately with the 'online_movable'
 	policy. Use this option to disable the automatic onlining behavior.
 
+-C::
+--check-config::
+	Get reconfiguration parameters from the global daxctl config file.
+	This is typically used when daxctl-reconfigure-device is called from
+	a systemd-udevd device unit file. The reconfiguration proceeds only
+	if the UUID of the dax device passed in on the command line matches
+	a UUID listed in the auto-online section of the config. See the
+	'PERSISTENT RECONFIGURATION' section for more details.
+
 include::movable-options.txt[]
 
 -f::
@@ -183,6 +192,64 @@ include::human-option.txt[]
 
 include::verbose-option.txt[]
 
+PERSISTENT RECONFIGURATION
+--------------------------
+
+The 'mode' of a daxctl device is not persistent across reboots by default. This
+is because the device itself may hold any metadata that hints at what mode it
+was set to, or is intended to be used in. The default mode for such a device
+is 'devdax', and on reboot, that is the mode devices appear in by default.
+
+The administrator may desire to configure the system in a way that certain
+dax devices are always reconfigured into a certain mode every time on boot.
+This is accomplished via a daxctl config file located at [location TBD].
+
+The config file may have multiple sections influencing different aspects of
+daxctl operation. The section of interest for persistent reconfiguration is
+'auto-online'. The format of this is as follows:
+
+----
+[auto-online <subsection_name>]
+uuid = <namespace uuid>
+mode = <desired reconfiguration mode> (default: system-ram)
+online = <true|false> (default: true)
+movable = <true|false> (default: true)
+----
+
+Here is an example of a config snippet for managing three devdax namespaces,
+one is left in devdax mode, the second is changed to system-ram mode with
+default options (online, movable), and the third is set to system-ram mode,
+the memory is onlined, but not movable.
+
+Note that the 'subsection name' can be arbitrary, and is only used to
+identify a specific config section. It does not have to match the 'device
+name' (e.g. 'dax0.0' etc).
+
+----
+[auto-online dax0]
+uuid = ed93e918-e165-49d8-921d-383d7b9660c5
+mode = devdax
+
+[auto-online dax1]
+uuid = f36d02ff-1d9f-4fb9-a5b9-8ceb10a00fe3
+mode = system-ram
+
+[auto-online dax2]
+uuid = f36d02ff-1d9f-4fb9-a5b9-8ceb10a00fe3
+mode = system-ram
+online = true
+movable = false
+----
+
+The following example can be used to create a devdax mode namespace, and
+simultaneously add the newly created namespace to the config file for
+system-ram conversion.
+
+----
+ndctl create-namespace --mode=devdax | \
+	jq -r "\"[auto-online $(uuidgen)]\", \"uuid = \(.uuid)\", \"mode = system-ram\"" >> $config_path
+----
+
 include::../copyright.txt[]
 
 SEE ALSO
-- 
2.31.1

[ndctl PATCH 3/7] util/parse-config: refactor filter_conf_files into util/

[Vishal Verma]

Move filter_conf() into util/parse-configs.c as filter_conf_files() so
that it can be reused by the config parser in daxctl.

Cc: QI Fuli <qi.fuli@fujitsu.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 ndctl/lib/libndctl.c   | 19 ++-----------------
 util/parse-configs.h   |  4 ++++
 util/parse-configs.c   | 16 ++++++++++++++++
 daxctl/lib/Makefile.am |  2 ++
 ndctl/lib/Makefile.am  |  2 ++
 5 files changed, 26 insertions(+), 17 deletions(-)

diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
index db2e38b..a01ac80 100644
--- a/ndctl/lib/libndctl.c
+++ b/ndctl/lib/libndctl.c
@@ -25,6 +25,7 @@
 #include <util/size.h>
 #include <util/sysfs.h>
 #include <util/strbuf.h>
+#include <util/parse-configs.h>
 #include <ndctl/libndctl.h>
 #include <ndctl/namespace.h>
 #include <daxctl/libdaxctl.h>
@@ -266,22 +267,6 @@ NDCTL_EXPORT void ndctl_set_userdata(struct ndctl_ctx *ctx, void *userdata)
 	ctx->userdata = userdata;
 }
 
-static int filter_conf(const struct dirent *dir)
-{
-	if (!dir)
-		return 0;
-
-	if (dir->d_type == DT_REG) {
-		const char *ext = strrchr(dir->d_name, '.');
-		if ((!ext) || (ext == dir->d_name))
-			return 0;
-		if (strcmp(ext, ".conf") == 0)
-			return 1;
-	}
-
-	return 0;
-}
-
 NDCTL_EXPORT void ndctl_set_configs(struct ndctl_ctx **ctx, char *conf_dir)
 {
 	struct dirent **namelist;
@@ -291,7 +276,7 @@ NDCTL_EXPORT void ndctl_set_configs(struct ndctl_ctx **ctx, char *conf_dir)
 	if ((!ctx) || (!conf_dir))
 		return;
 
-	rc = scandir(conf_dir, &namelist, filter_conf, alphasort);
+	rc = scandir(conf_dir, &namelist, filter_conf_files, alphasort);
 	if (rc == -1) {
 		perror("scandir");
 		return;
diff --git a/util/parse-configs.h b/util/parse-configs.h
index f70f58f..491aebb 100644
--- a/util/parse-configs.h
+++ b/util/parse-configs.h
@@ -1,8 +1,10 @@
 // SPDX-License-Identifier: GPL-2.0
 // Copyright (C) 2021, FUJITSU LIMITED. ALL rights reserved.
 
+#include <dirent.h>
 #include <stdbool.h>
 #include <stdint.h>
+#include <string.h>
 #include <util/util.h>
 
 enum parse_conf_type {
@@ -11,6 +13,8 @@ enum parse_conf_type {
 	MONITOR_CALLBACK,
 };
 
+int filter_conf_files(const struct dirent *dir);
+
 struct config;
 typedef int parse_conf_cb(const struct config *, const char *config_file);
 
diff --git a/util/parse-configs.c b/util/parse-configs.c
index 44dcff4..72c4913 100644
--- a/util/parse-configs.c
+++ b/util/parse-configs.c
@@ -6,6 +6,22 @@
 #include <util/strbuf.h>
 #include <util/iniparser.h>
 
+int filter_conf_files(const struct dirent *dir)
+{
+	if (!dir)
+		return 0;
+
+	if (dir->d_type == DT_REG) {
+		const char *ext = strrchr(dir->d_name, '.');
+		if ((!ext) || (ext == dir->d_name))
+			return 0;
+		if (strcmp(ext, ".conf") == 0)
+			return 1;
+	}
+
+	return 0;
+}
+
 static void set_str_val(const char **value, const char *val)
 {
 	struct strbuf buf = STRBUF_INIT;
diff --git a/daxctl/lib/Makefile.am b/daxctl/lib/Makefile.am
index 25efd83..db2351e 100644
--- a/daxctl/lib/Makefile.am
+++ b/daxctl/lib/Makefile.am
@@ -15,6 +15,8 @@ libdaxctl_la_SOURCES =\
 	../../util/sysfs.h \
 	../../util/log.c \
 	../../util/log.h \
+	../../util/parse-configs.h \
+	../../util/parse-configs.c \
 	libdaxctl.c
 
 libdaxctl_la_LIBADD =\
diff --git a/ndctl/lib/Makefile.am b/ndctl/lib/Makefile.am
index f741c44..8020eb4 100644
--- a/ndctl/lib/Makefile.am
+++ b/ndctl/lib/Makefile.am
@@ -19,6 +19,8 @@ libndctl_la_SOURCES =\
 	../../util/wrapper.c \
 	../../util/usage.c \
 	../../util/fletcher.h \
+	../../util/parse-configs.h \
+	../../util/parse-configs.c \
 	dimm.c \
 	inject.c \
 	nfit.c \
-- 
2.31.1

[ndctl PATCH 4/7] daxctl: add basic config parsing support

[Vishal Verma]

Add support similar to ndctl and libndctl for parsing config files. This
allows storing a config file path/list in the daxctl_ctx, and adds APIs
for setting and retrieving it.

Cc: QI Fuli <qi.fuli@fujitsu.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 daxctl/lib/libdaxctl.c   | 37 +++++++++++++++++++++++++++++++++++++
 daxctl/libdaxctl.h       |  2 ++
 daxctl/Makefile.am       |  1 +
 daxctl/lib/Makefile.am   |  4 ++++
 daxctl/lib/libdaxctl.sym |  2 ++
 5 files changed, 46 insertions(+)

diff --git a/daxctl/lib/libdaxctl.c b/daxctl/lib/libdaxctl.c
index 860bd9c..659d2fe 100644
--- a/daxctl/lib/libdaxctl.c
+++ b/daxctl/lib/libdaxctl.c
@@ -17,6 +17,8 @@
 #include <util/log.h>
 #include <util/sysfs.h>
 #include <util/iomem.h>
+#include <util/strbuf.h>
+#include <util/parse-configs.h>
 #include <daxctl/libdaxctl.h>
 #include "libdaxctl-private.h"
 
@@ -37,6 +39,7 @@ struct daxctl_ctx {
 	struct log_ctx ctx;
 	int refcount;
 	void *userdata;
+	const char *configs;
 	int regions_init;
 	struct list_head regions;
 	struct kmod_ctx *kmod_ctx;
@@ -68,6 +71,40 @@ DAXCTL_EXPORT void daxctl_set_userdata(struct daxctl_ctx *ctx, void *userdata)
 	ctx->userdata = userdata;
 }
 
+DAXCTL_EXPORT void daxctl_set_configs(struct daxctl_ctx **ctx, char *conf_dir)
+{
+	struct dirent **namelist;
+	struct strbuf value = STRBUF_INIT;
+	int rc;
+
+	if ((!ctx) || (!conf_dir))
+		return;
+
+	rc = scandir(conf_dir, &namelist, filter_conf_files, alphasort);
+	if (rc == -1) {
+		perror("scandir");
+		return;
+	}
+
+	while (rc--) {
+		if (value.len)
+			strbuf_addstr(&value, " ");
+		strbuf_addstr(&value, conf_dir);
+		strbuf_addstr(&value, "/");
+		strbuf_addstr(&value, namelist[rc]->d_name);
+		free(namelist[rc]);
+	}
+	(*ctx)->configs = strbuf_detach(&value, NULL);
+	free(namelist);
+}
+
+DAXCTL_EXPORT const char *daxctl_get_configs(struct daxctl_ctx *ctx)
+{
+	if (ctx == NULL)
+		return NULL;
+	return ctx->configs;
+}
+
 /**
  * daxctl_new - instantiate a new library context
  * @ctx: context to establish
diff --git a/daxctl/libdaxctl.h b/daxctl/libdaxctl.h
index 683ae9c..9388f85 100644
--- a/daxctl/libdaxctl.h
+++ b/daxctl/libdaxctl.h
@@ -28,6 +28,8 @@ int daxctl_get_log_priority(struct daxctl_ctx *ctx);
 void daxctl_set_log_priority(struct daxctl_ctx *ctx, int priority);
 void daxctl_set_userdata(struct daxctl_ctx *ctx, void *userdata);
 void *daxctl_get_userdata(struct daxctl_ctx *ctx);
+void daxctl_set_configs(struct daxctl_ctx **ctx, char *conf_dir);
+const char *daxctl_get_configs(struct daxctl_ctx *ctx);
 
 struct daxctl_region;
 struct daxctl_region *daxctl_new_region(struct daxctl_ctx *ctx, int id,
diff --git a/daxctl/Makefile.am b/daxctl/Makefile.am
index 9b1313a..a9845a0 100644
--- a/daxctl/Makefile.am
+++ b/daxctl/Makefile.am
@@ -10,6 +10,7 @@ config.h: $(srcdir)/Makefile.am
 		"$(daxctl_modprobe_datadir)/$(daxctl_modprobe_data)"' >>$@ && \
 	echo '#define DAXCTL_MODPROBE_INSTALL \
 		"$(sysconfdir)/modprobe.d/$(daxctl_modprobe_data)"' >>$@
+	$(AM_V_GEN) echo '#define DAXCTL_CONF_DIR  "$(ndctl_confdir)"' >>$@
 
 daxctl_SOURCES =\
 		daxctl.c \
diff --git a/daxctl/lib/Makefile.am b/daxctl/lib/Makefile.am
index db2351e..7a53598 100644
--- a/daxctl/lib/Makefile.am
+++ b/daxctl/lib/Makefile.am
@@ -13,6 +13,10 @@ libdaxctl_la_SOURCES =\
 	../../util/iomem.h \
 	../../util/sysfs.c \
 	../../util/sysfs.h \
+	../../util/strbuf.h \
+	../../util/strbuf.c \
+	../../util/wrapper.c \
+	../../util/usage.c \
 	../../util/log.c \
 	../../util/log.h \
 	../../util/parse-configs.h \
diff --git a/daxctl/lib/libdaxctl.sym b/daxctl/lib/libdaxctl.sym
index a13e93d..190b605 100644
--- a/daxctl/lib/libdaxctl.sym
+++ b/daxctl/lib/libdaxctl.sym
@@ -96,4 +96,6 @@ LIBDAXCTL_9 {
 global:
 	daxctl_dev_will_auto_online_memory;
 	daxctl_dev_has_online_memory;
+	daxctl_set_configs;
+	daxctl_get_configs;
 } LIBDAXCTL_8;
-- 
2.31.1

[ndctl PATCH 5/7] util/parse-configs: add a key/value search helper

[Vishal Verma]

Add a new config query type called CONFIG_SEARCH_SECTION, which searches
all loaded config files based on a query criteria of: specified section
name, specified key/value pair within that section, and can return other
key/values from the section that matched the search criteria.

This allows for multiple named subsections, where a subsection name is
of the type: '[section subsection]'.

Cc: QI Fuli <qi.fuli@fujitsu.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 util/parse-configs.h | 15 +++++++++++++
 util/parse-configs.c | 51 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+)

diff --git a/util/parse-configs.h b/util/parse-configs.h
index 491aebb..6dcc01c 100644
--- a/util/parse-configs.h
+++ b/util/parse-configs.h
@@ -9,6 +9,7 @@
 
 enum parse_conf_type {
 	CONFIG_STRING,
+	CONFIG_SEARCH_SECTION,
 	CONFIG_END,
 	MONITOR_CALLBACK,
 };
@@ -20,6 +21,10 @@ typedef int parse_conf_cb(const struct config *, const char *config_file);
 
 struct config {
 	enum parse_conf_type type;
+	const char *section;
+	const char *search_key;
+	const char *search_val;
+	const char *get_key;
 	const char *key;
 	void *value;
 	void *defval;
@@ -31,6 +36,16 @@ struct config {
 #define CONF_END() { .type = CONFIG_END }
 #define CONF_STR(k,v,d) \
 	{ .type = CONFIG_STRING, .key = (k), .value = check_vtype(v, const char **), .defval = (d) }
+#define CONF_SEARCH(s, sk, sv, gk, v, d)	\
+{						\
+	.type = CONFIG_SEARCH_SECTION,		\
+	.section = (s),				\
+	.search_key = (sk),			\
+	.search_val = (sv),			\
+	.get_key = (gk),			\
+	.value = check_vtype(v, const char **),	\
+	.defval = (d)				\
+}
 #define CONF_MONITOR(k,f) \
 	{ .type = MONITOR_CALLBACK, .key = (k), .callback = (f)}
 
diff --git a/util/parse-configs.c b/util/parse-configs.c
index 72c4913..8eabe3d 100644
--- a/util/parse-configs.c
+++ b/util/parse-configs.c
@@ -38,6 +38,54 @@ static void set_str_val(const char **value, const char *val)
 	*value = strbuf_detach(&buf, NULL);
 }
 
+static const char *search_section_kv(dictionary *d, const struct config *c)
+{
+	int i;
+
+	for (i = 0; i < iniparser_getnsec(d); i++) {
+		const char *cur_sec_full = iniparser_getsecname(d, i);
+		char *cur_sec = strdup(cur_sec_full);
+		const char *search_val, *ret_val;
+		const char *delim = " \t\n\r";
+		char *save, *cur, *query;
+
+		if (!cur_sec)
+			return NULL;
+		if (!c->section || !c->search_key || !c->search_val || !c->get_key) {
+			fprintf(stderr, "warning: malformed config query, skipping\n");
+			return NULL;
+		}
+
+		cur = strtok_r(cur_sec, delim, &save);
+		if ((cur == NULL) || (strcmp(cur, c->section) != 0))
+			goto out_sec;
+
+		if (asprintf(&query, "%s:%s", cur_sec_full, c->search_key) < 0)
+			goto out_sec;
+		search_val = iniparser_getstring(d, query, NULL);
+		if (!search_val)
+			goto out_query;
+		if (strcmp(search_val, c->search_val) != 0)
+			goto out_query;
+
+		/* we're now in a matching section */
+		free(query);
+		if (asprintf(&query, "%s:%s", cur_sec_full, c->get_key) < 0)
+			goto out_sec;
+		ret_val = iniparser_getstring(d, query, NULL);
+		free(query);
+		free(cur_sec);
+		return ret_val;
+
+out_query:
+		free(query);
+out_sec:
+		free(cur_sec);
+	}
+
+	return NULL;
+}
+
 static int parse_config_file(const char *config_file,
 			const struct config *configs)
 {
@@ -54,6 +102,9 @@ static int parse_config_file(const char *config_file,
 					iniparser_getstring(dic,
 					configs->key, configs->defval));
 			break;
+		case CONFIG_SEARCH_SECTION:
+			set_str_val((const char **)configs->value,
+					search_section_kv(dic, configs));
 		case MONITOR_CALLBACK:
 		case CONFIG_END:
 			break;
-- 
2.31.1

[ndctl PATCH 6/7] daxctl/device.c: add an option for getting params from a config file

[Vishal Verma]

Add a new option to daxctl-reconfigure-device that allows it to
comprehend the new global config system in ndctl/daxctl. With this, the
reconfigure-device command can query the config to match a specific
device UUID, and operate using the parameters supplied in that INI
section.

This is in preparation to make daxctl device reconfiguration (usually
as system-ram) policy based, so that reconfiguration can happen
automatically on boot.

Cc: QI Fuli <qi.fuli@fujitsu.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 daxctl/daxctl.c    |   1 +
 daxctl/device.c    | 141 ++++++++++++++++++++++++++++++++++++++++++++-
 daxctl/Makefile.am |   1 +
 3 files changed, 142 insertions(+), 1 deletion(-)

diff --git a/daxctl/daxctl.c b/daxctl/daxctl.c
index 928814c..dc7ac5f 100644
--- a/daxctl/daxctl.c
+++ b/daxctl/daxctl.c
@@ -95,6 +95,7 @@ int main(int argc, const char **argv)
 	rc = daxctl_new(&ctx);
 	if (rc)
 		goto out;
+	daxctl_set_configs(&ctx, DAXCTL_CONF_DIR);
 	main_handle_internal_command(argc, argv, ctx, commands,
 			ARRAY_SIZE(commands), PROG_DAXCTL);
 	daxctl_unref(ctx);
diff --git a/daxctl/device.c b/daxctl/device.c
index a427b7d..99a4548 100644
--- a/daxctl/device.c
+++ b/daxctl/device.c
@@ -14,8 +14,10 @@
 #include <util/filter.h>
 #include <json-c/json.h>
 #include <json-c/json_util.h>
+#include <ndctl/libndctl.h>
 #include <daxctl/libdaxctl.h>
 #include <util/parse-options.h>
+#include <util/parse-configs.h>
 #include <ccan/array_size/array_size.h>
 
 static struct {
@@ -25,6 +27,7 @@ static struct {
 	const char *size;
 	const char *align;
 	const char *input;
+	bool check_config;
 	bool no_online;
 	bool no_movable;
 	bool force;
@@ -75,7 +78,9 @@ OPT_STRING('m', "mode", &param.mode, "mode", "mode to switch the device to"), \
 OPT_BOOLEAN('N', "no-online", &param.no_online, \
 	"don't auto-online memory sections"), \
 OPT_BOOLEAN('f', "force", &param.force, \
-		"attempt to offline memory sections before reconfiguration")
+		"attempt to offline memory sections before reconfiguration"), \
+OPT_BOOLEAN('C', "check-config", &param.check_config, \
+		"use config files to determine parameters for the operation")
 
 #define CREATE_OPTIONS() \
 OPT_STRING('s', "size", &param.size, "size", "size to switch the device to"), \
@@ -218,6 +223,130 @@ err:
 	return rc;
 }
 
+static int conf_string_to_bool(const char *str)
+{
+	if (!str)
+		return INT_MAX;
+	if (strncmp(str, "t", 1) == 0 ||
+			strncmp(str, "T", 1) == 0 ||
+			strncmp(str, "y", 1) == 0 ||
+			strncmp(str, "Y", 1) == 0 ||
+			strncmp(str, "1", 1) == 0)
+		return true;
+	if (strncmp(str, "f", 1) == 0 ||
+			strncmp(str, "F", 1) == 0 ||
+			strncmp(str, "n", 1) == 0 ||
+			strncmp(str, "N", 1) == 0 ||
+			strncmp(str, "0", 1) == 0)
+		return false;
+	return INT_MAX;
+}
+
+#define conf_assign_inverted_bool(p, conf_var) \
+do { \
+	if (conf_string_to_bool(conf_var) != INT_MAX) \
+		param.p = !conf_string_to_bool(conf_var); \
+} while(0)
+
+static int parse_config_reconfig_set_params(struct daxctl_ctx *ctx, const char *device,
+					    const char *uuid)
+{
+	const char *conf_online = NULL, *conf_movable = NULL;
+	const struct config configs[] = {
+		CONF_SEARCH("auto-online", "uuid", uuid, "mode", &param.mode, NULL),
+		CONF_SEARCH("auto-online", "uuid", uuid, "online", &conf_online, NULL),
+		CONF_SEARCH("auto-online", "uuid", uuid, "movable", &conf_movable, NULL),
+		CONF_END(),
+	};
+	const char *prefix = "./";
+	int rc;
+
+	rc = parse_configs_prefix(daxctl_get_configs(ctx), prefix, configs);
+	if (rc < 0)
+		return rc;
+
+	conf_assign_inverted_bool(no_online, conf_online);
+	conf_assign_inverted_bool(no_movable, conf_movable);
+
+	return 0;
+}
+
+static bool daxctl_ndns_has_device(struct ndctl_namespace *ndns,
+				    const char *device)
+{
+	struct daxctl_region *dax_region;
+	struct ndctl_dax *dax;
+
+	dax = ndctl_namespace_get_dax(ndns);
+	if (!dax)
+		return false;
+
+	dax_region = ndctl_dax_get_daxctl_region(dax);
+	if (dax_region) {
+		struct daxctl_dev *dev;
+
+		dev = daxctl_dev_get_first(dax_region);
+		if (dev) {
+			if (strcmp(daxctl_dev_get_devname(dev), device) == 0)
+				return true;
+		}
+	}
+	return false;
+}
+
+static int parse_config_reconfig(struct daxctl_ctx *ctx, const char *device)
+{
+	struct ndctl_namespace *ndns;
+	struct ndctl_ctx *ndctl_ctx;
+	struct ndctl_region *region;
+	struct ndctl_bus *bus;
+	struct ndctl_dax *dax;
+	int rc, found = 0;
+	char uuid_buf[40];
+	uuid_t uuid;
+
+	if (strcmp(device, "all") == 0)
+		return 0;
+
+	rc = ndctl_new(&ndctl_ctx);
+	if (rc < 0)
+		return rc;
+
+        ndctl_bus_foreach(ndctl_ctx, bus) {
+		ndctl_region_foreach(bus, region) {
+			ndctl_namespace_foreach(region, ndns) {
+				if (daxctl_ndns_has_device(ndns, device)) {
+					dax = ndctl_namespace_get_dax(ndns);
+					if (!dax)
+						continue;
+					ndctl_dax_get_uuid(dax, uuid);
+					found = 1;
+				}
+			}
+		}
+	}
+
+	if (!found) {
+		fprintf(stderr, "no UUID match for %s found in config files\n",
+			device);
+		return 0;
+	}
+
+	uuid_unparse(uuid, uuid_buf);
+	return parse_config_reconfig_set_params(ctx, device, uuid_buf);
+}
+
+static int parse_device_config(struct daxctl_ctx *ctx, const char *device,
+			       enum device_action action)
+{
+	switch (action) {
+	case ACTION_RECONFIG:
+		return parse_config_reconfig(ctx, device);
+	default:
+		return 0;
+	}
+}
+
 static const char *parse_device_options(int argc, const char **argv,
 		enum device_action action, const struct option *options,
 		const char *usage, struct daxctl_ctx *ctx)
@@ -279,6 +408,16 @@ static const char *parse_device_options(int argc, const char **argv,
 	if (param.human)
 		flags |= UTIL_JSON_HUMAN;
 
+	/* Scan config file(s) for options. This sets param.foo accordingly */
+	if (param.check_config) {
+		rc = parse_device_config(ctx, argv[0], action);
+		if (rc) {
+			fprintf(stderr, "error parsing config file: %s\n",
+				strerror(-rc));
+			return NULL;
+		}
+	}
+
 	/* Handle action-specific options */
 	switch (action) {
 	case ACTION_RECONFIG:
diff --git a/daxctl/Makefile.am b/daxctl/Makefile.am
index a9845a0..f30c485 100644
--- a/daxctl/Makefile.am
+++ b/daxctl/Makefile.am
@@ -23,6 +23,7 @@ daxctl_SOURCES =\
 
 daxctl_LDADD =\
 	lib/libdaxctl.la \
+	../ndctl/lib/libndctl.la \
 	../libutil.a \
 	$(UUID_LIBS) \
 	$(KMOD_LIBS) \
-- 
2.31.1

[ndctl PATCH 7/7] daxctl: add systemd service and udev rule for auto-onlining

[Vishal Verma]

Install a helper script that calls daxctl-reconfigure-device with the
new 'check-config' option for a given device. This is meant to be called
via a systemd service.

Install a systemd service that calls the above wrapper script with a
daxctl device passed in to it via the environment.

Install a udev rule that is triggered for every daxctl device, and
triggers the above oneshot systemd service.

Together, these three things work such that upon boot, whenever a daxctl
device is found, udev triggers a device-specific systemd service called,
for example:

  daxdev-reconfigure@-dev-dax0.0.service

This initiates a daxctl-reconfigure-device with a config lookup for the
'dax0.0' device. If the config has an '[auto-online <unique_id>]'
section, it uses the information in that to set the operating mode of
the device.

If any device is in an unexpected status, 'journalctl' can be used to
view the reconfiguration log for that device, for example:

  journalctl --unit daxdev-reconfigure@-dev-dax0.0.service

Update the RPM spec file to include the newly added files to the RPM
build.

Cc: QI Fuli <qi.fuli@fujitsu.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 configure.ac                       |  9 ++++++++-
 daxctl/90-daxctl-device.rules      |  1 +
 daxctl/Makefile.am                 | 10 ++++++++++
 daxctl/daxdev-auto-reconfigure.sh  |  3 +++
 daxctl/daxdev-reconfigure@.service |  8 ++++++++
 ndctl.spec.in                      |  3 +++
 6 files changed, 33 insertions(+), 1 deletion(-)
 create mode 100644 daxctl/90-daxctl-device.rules
 create mode 100755 daxctl/daxdev-auto-reconfigure.sh
 create mode 100644 daxctl/daxdev-reconfigure@.service

diff --git a/configure.ac b/configure.ac
index 9e1c6db..df6ab10 100644
--- a/configure.ac
+++ b/configure.ac
@@ -160,7 +160,7 @@ AC_CHECK_FUNCS([ \
 
 AC_ARG_WITH([systemd],
 	AS_HELP_STRING([--with-systemd],
-		[Enable systemd functionality (monitor). @<:@default=yes@:>@]),
+		[Enable systemd functionality. @<:@default=yes@:>@]),
 	[], [with_systemd=yes])
 
 if test "x$with_systemd" = "xyes"; then
@@ -183,6 +183,13 @@ daxctl_modprobe_data=daxctl.conf
 AC_SUBST([daxctl_modprobe_datadir])
 AC_SUBST([daxctl_modprobe_data])
 
+AC_ARG_WITH(udevrulesdir,
+    [AS_HELP_STRING([--with-udevrulesdir=DIR], [udev rules.d directory])],
+    [UDEVRULESDIR="$withval"],
+    [UDEVRULESDIR='${prefix}/lib/udev/rules.d']
+)
+AC_SUBST(UDEVRULESDIR)
+
 AC_ARG_WITH([keyutils],
 	    AS_HELP_STRING([--with-keyutils],
 			[Enable keyutils functionality (security).  @<:@default=yes@:>@]), [], [with_keyutils=yes])
diff --git a/daxctl/90-daxctl-device.rules b/daxctl/90-daxctl-device.rules
new file mode 100644
index 0000000..ee0670f
--- /dev/null
+++ b/daxctl/90-daxctl-device.rules
@@ -0,0 +1 @@
+ACTION=="add", SUBSYSTEM=="dax", TAG+="systemd", ENV{SYSTEMD_WANTS}="daxdev-reconfigure@$env{DEVNAME}.service"
diff --git a/daxctl/Makefile.am b/daxctl/Makefile.am
index f30c485..d53bdcf 100644
--- a/daxctl/Makefile.am
+++ b/daxctl/Makefile.am
@@ -28,3 +28,13 @@ daxctl_LDADD =\
 	$(UUID_LIBS) \
 	$(KMOD_LIBS) \
 	$(JSON_LIBS)
+
+bin_SCRIPTS = daxdev-auto-reconfigure.sh
+CLEANFILES = $(bin_SCRIPTS)
+
+udevrulesdir = $(UDEVRULESDIR)
+udevrules_DATA = 90-daxctl-device.rules
+
+if ENABLE_SYSTEMD_UNITS
+systemd_unit_DATA = daxdev-reconfigure@.service
+endif
diff --git a/daxctl/daxdev-auto-reconfigure.sh b/daxctl/daxdev-auto-reconfigure.sh
new file mode 100755
index 0000000..f6da43f
--- /dev/null
+++ b/daxctl/daxdev-auto-reconfigure.sh
@@ -0,0 +1,3 @@
+#!/bin/bash
+
+daxctl reconfigure-device --check-config "${1##*/}"
diff --git a/daxctl/daxdev-reconfigure@.service b/daxctl/daxdev-reconfigure@.service
new file mode 100644
index 0000000..451fef1
--- /dev/null
+++ b/daxctl/daxdev-reconfigure@.service
@@ -0,0 +1,8 @@
+[Unit]
+Description=Automatic daxctl device reconfiguration
+Documentation=man:daxctl-reconfigure-device(1)
+
+[Service]
+Type=forking
+GuessMainPID=false
+ExecStart=/bin/sh -c "exec daxdev-auto-reconfigure.sh %I"
diff --git a/ndctl.spec.in b/ndctl.spec.in
index 07c36ec..fd1a5ff 100644
--- a/ndctl.spec.in
+++ b/ndctl.spec.in
@@ -124,8 +124,11 @@ make check
 %defattr(-,root,root)
 %license LICENSES/preferred/GPL-2.0 LICENSES/other/MIT LICENSES/other/CC0-1.0
 %{_bindir}/daxctl
+%{_bindir}/daxdev-auto-reconfigure.sh
 %{_mandir}/man1/daxctl*
 %{_datadir}/daxctl/daxctl.conf
+%{_unitdir}/daxdev-reconfigure@.service
+%config %{_udevrulesdir}/90-daxctl-device.rules
 
 %files -n LNAME
 %defattr(-,root,root)
-- 
2.31.1

RE: [ndctl PATCH 1/7] ndctl: Update ndctl.spec.in for 'ndctl.conf'

[qi.fuli]

> Subject: [ndctl PATCH 1/7] ndctl: Update ndctl.spec.in for 'ndctl.conf'
> 
> The new config system introduces and installs a sample config file called
> ndctl.conf. Update the RPM spec to include this in the %files section for ndctl.
> 
> Cc: QI Fuli <qi.fuli@fujitsu.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  ndctl.spec.in | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/ndctl.spec.in b/ndctl.spec.in index 0563b2d..07c36ec 100644
> --- a/ndctl.spec.in
> +++ b/ndctl.spec.in
> @@ -118,6 +118,7 @@ make check
>  %{_sysconfdir}/modprobe.d/nvdimm-security.conf
> 
>  %config(noreplace) %{_sysconfdir}/ndctl/monitor.conf
> +%config(noreplace) %{_sysconfdir}/ndctl/ndctl.conf
> 
>  %files -n daxctl
>  %defattr(-,root,root)
> --
> 2.31.1

Looks good to me.
Thank you very much.

QI

RE: [ndctl PATCH 3/7] util/parse-config: refactor filter_conf_files into util/

[qi.fuli]

> Subject: [ndctl PATCH 3/7] util/parse-config: refactor filter_conf_files into util/
> 
> Move filter_conf() into util/parse-configs.c as filter_conf_files() so that it can be
> reused by the config parser in daxctl.
> 
> Cc: QI Fuli <qi.fuli@fujitsu.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  ndctl/lib/libndctl.c   | 19 ++-----------------
>  util/parse-configs.h   |  4 ++++
>  util/parse-configs.c   | 16 ++++++++++++++++
>  daxctl/lib/Makefile.am |  2 ++
>  ndctl/lib/Makefile.am  |  2 ++
>  5 files changed, 26 insertions(+), 17 deletions(-)
> 
> diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c index db2e38b..a01ac80
> 100644
> --- a/ndctl/lib/libndctl.c
> +++ b/ndctl/lib/libndctl.c
> @@ -25,6 +25,7 @@
>  #include <util/size.h>
>  #include <util/sysfs.h>
>  #include <util/strbuf.h>
> +#include <util/parse-configs.h>
>  #include <ndctl/libndctl.h>
>  #include <ndctl/namespace.h>
>  #include <daxctl/libdaxctl.h>
> @@ -266,22 +267,6 @@ NDCTL_EXPORT void ndctl_set_userdata(struct
> ndctl_ctx *ctx, void *userdata)
>  	ctx->userdata = userdata;
>  }
> 
> -static int filter_conf(const struct dirent *dir) -{
> -	if (!dir)
> -		return 0;
> -
> -	if (dir->d_type == DT_REG) {
> -		const char *ext = strrchr(dir->d_name, '.');
> -		if ((!ext) || (ext == dir->d_name))
> -			return 0;
> -		if (strcmp(ext, ".conf") == 0)
> -			return 1;
> -	}
> -
> -	return 0;
> -}
> -
>  NDCTL_EXPORT void ndctl_set_configs(struct ndctl_ctx **ctx, char *conf_dir)
> {
>  	struct dirent **namelist;
> @@ -291,7 +276,7 @@ NDCTL_EXPORT void ndctl_set_configs(struct ndctl_ctx
> **ctx, char *conf_dir)
>  	if ((!ctx) || (!conf_dir))
>  		return;
> 
> -	rc = scandir(conf_dir, &namelist, filter_conf, alphasort);
> +	rc = scandir(conf_dir, &namelist, filter_conf_files, alphasort);
>  	if (rc == -1) {
>  		perror("scandir");
>  		return;
> diff --git a/util/parse-configs.h b/util/parse-configs.h index f70f58f..491aebb
> 100644
> --- a/util/parse-configs.h
> +++ b/util/parse-configs.h
> @@ -1,8 +1,10 @@
>  // SPDX-License-Identifier: GPL-2.0
>  // Copyright (C) 2021, FUJITSU LIMITED. ALL rights reserved.
> 
> +#include <dirent.h>
>  #include <stdbool.h>
>  #include <stdint.h>
> +#include <string.h>
>  #include <util/util.h>
> 
>  enum parse_conf_type {
> @@ -11,6 +13,8 @@ enum parse_conf_type {
>  	MONITOR_CALLBACK,
>  };
> 
> +int filter_conf_files(const struct dirent *dir);
> +
>  struct config;
>  typedef int parse_conf_cb(const struct config *, const char *config_file);
> 
> diff --git a/util/parse-configs.c b/util/parse-configs.c index 44dcff4..72c4913
> 100644
> --- a/util/parse-configs.c
> +++ b/util/parse-configs.c
> @@ -6,6 +6,22 @@
>  #include <util/strbuf.h>
>  #include <util/iniparser.h>
> 
> +int filter_conf_files(const struct dirent *dir) {
> +	if (!dir)
> +		return 0;
> +
> +	if (dir->d_type == DT_REG) {
> +		const char *ext = strrchr(dir->d_name, '.');
> +		if ((!ext) || (ext == dir->d_name))
> +			return 0;
> +		if (strcmp(ext, ".conf") == 0)
> +			return 1;
> +	}
> +
> +	return 0;
> +}
> +
>  static void set_str_val(const char **value, const char *val)  {
>  	struct strbuf buf = STRBUF_INIT;
> diff --git a/daxctl/lib/Makefile.am b/daxctl/lib/Makefile.am index
> 25efd83..db2351e 100644
> --- a/daxctl/lib/Makefile.am
> +++ b/daxctl/lib/Makefile.am
> @@ -15,6 +15,8 @@ libdaxctl_la_SOURCES =\
>  	../../util/sysfs.h \
>  	../../util/log.c \
>  	../../util/log.h \
> +	../../util/parse-configs.h \
> +	../../util/parse-configs.c \
>  	libdaxctl.c
> 
>  libdaxctl_la_LIBADD =\
> diff --git a/ndctl/lib/Makefile.am b/ndctl/lib/Makefile.am index
> f741c44..8020eb4 100644
> --- a/ndctl/lib/Makefile.am
> +++ b/ndctl/lib/Makefile.am
> @@ -19,6 +19,8 @@ libndctl_la_SOURCES =\
>  	../../util/wrapper.c \
>  	../../util/usage.c \
>  	../../util/fletcher.h \
> +	../../util/parse-configs.h \
> +	../../util/parse-configs.c \
>  	dimm.c \
>  	inject.c \
>  	nfit.c \
> --
> 2.31.1

Looks good to me.
Thank you very much.

QI

RE: [ndctl PATCH 4/7] daxctl: add basic config parsing support

[qi.fuli]

> Subject: [ndctl PATCH 4/7] daxctl: add basic config parsing support
> 
> Add support similar to ndctl and libndctl for parsing config files. This allows
> storing a config file path/list in the daxctl_ctx, and adds APIs for setting and
> retrieving it.
> 
> Cc: QI Fuli <qi.fuli@fujitsu.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  daxctl/lib/libdaxctl.c   | 37
> +++++++++++++++++++++++++++++++++++++
>  daxctl/libdaxctl.h       |  2 ++
>  daxctl/Makefile.am       |  1 +
>  daxctl/lib/Makefile.am   |  4 ++++
>  daxctl/lib/libdaxctl.sym |  2 ++
>  5 files changed, 46 insertions(+)
> 
> diff --git a/daxctl/lib/libdaxctl.c b/daxctl/lib/libdaxctl.c index 860bd9c..659d2fe
> 100644
> --- a/daxctl/lib/libdaxctl.c
> +++ b/daxctl/lib/libdaxctl.c
> @@ -17,6 +17,8 @@
>  #include <util/log.h>
>  #include <util/sysfs.h>
>  #include <util/iomem.h>
> +#include <util/strbuf.h>
> +#include <util/parse-configs.h>
>  #include <daxctl/libdaxctl.h>
>  #include "libdaxctl-private.h"
> 
> @@ -37,6 +39,7 @@ struct daxctl_ctx {
>  	struct log_ctx ctx;
>  	int refcount;
>  	void *userdata;
> +	const char *configs;
>  	int regions_init;
>  	struct list_head regions;
>  	struct kmod_ctx *kmod_ctx;
> @@ -68,6 +71,40 @@ DAXCTL_EXPORT void daxctl_set_userdata(struct
> daxctl_ctx *ctx, void *userdata)
>  	ctx->userdata = userdata;
>  }
> 
> +DAXCTL_EXPORT void daxctl_set_configs(struct daxctl_ctx **ctx, char
> +*conf_dir) {
> +	struct dirent **namelist;
> +	struct strbuf value = STRBUF_INIT;
> +	int rc;
> +
> +	if ((!ctx) || (!conf_dir))
> +		return;
> +
> +	rc = scandir(conf_dir, &namelist, filter_conf_files, alphasort);
> +	if (rc == -1) {
> +		perror("scandir");
> +		return;
> +	}
> +
> +	while (rc--) {
> +		if (value.len)
> +			strbuf_addstr(&value, " ");
> +		strbuf_addstr(&value, conf_dir);
> +		strbuf_addstr(&value, "/");
> +		strbuf_addstr(&value, namelist[rc]->d_name);
> +		free(namelist[rc]);
> +	}
> +	(*ctx)->configs = strbuf_detach(&value, NULL);
> +	free(namelist);
> +}
> +
> +DAXCTL_EXPORT const char *daxctl_get_configs(struct daxctl_ctx *ctx) {
> +	if (ctx == NULL)
> +		return NULL;
> +	return ctx->configs;
> +}
> +
>  /**
>   * daxctl_new - instantiate a new library context
>   * @ctx: context to establish
> diff --git a/daxctl/libdaxctl.h b/daxctl/libdaxctl.h index 683ae9c..9388f85 100644
> --- a/daxctl/libdaxctl.h
> +++ b/daxctl/libdaxctl.h
> @@ -28,6 +28,8 @@ int daxctl_get_log_priority(struct daxctl_ctx *ctx);  void
> daxctl_set_log_priority(struct daxctl_ctx *ctx, int priority);  void
> daxctl_set_userdata(struct daxctl_ctx *ctx, void *userdata);  void
> *daxctl_get_userdata(struct daxctl_ctx *ctx);
> +void daxctl_set_configs(struct daxctl_ctx **ctx, char *conf_dir); const
> +char *daxctl_get_configs(struct daxctl_ctx *ctx);
> 
>  struct daxctl_region;
>  struct daxctl_region *daxctl_new_region(struct daxctl_ctx *ctx, int id, diff --git
> a/daxctl/Makefile.am b/daxctl/Makefile.am index 9b1313a..a9845a0 100644
> --- a/daxctl/Makefile.am
> +++ b/daxctl/Makefile.am
> @@ -10,6 +10,7 @@ config.h: $(srcdir)/Makefile.am
>  		"$(daxctl_modprobe_datadir)/$(daxctl_modprobe_data)"' >>$@
> && \
>  	echo '#define DAXCTL_MODPROBE_INSTALL \
>  		"$(sysconfdir)/modprobe.d/$(daxctl_modprobe_data)"' >>$@
> +	$(AM_V_GEN) echo '#define DAXCTL_CONF_DIR  "$(ndctl_confdir)"'
> >>$@
> 
>  daxctl_SOURCES =\
>  		daxctl.c \
> diff --git a/daxctl/lib/Makefile.am b/daxctl/lib/Makefile.am index
> db2351e..7a53598 100644
> --- a/daxctl/lib/Makefile.am
> +++ b/daxctl/lib/Makefile.am
> @@ -13,6 +13,10 @@ libdaxctl_la_SOURCES =\
>  	../../util/iomem.h \
>  	../../util/sysfs.c \
>  	../../util/sysfs.h \
> +	../../util/strbuf.h \
> +	../../util/strbuf.c \
> +	../../util/wrapper.c \
> +	../../util/usage.c \
>  	../../util/log.c \
>  	../../util/log.h \
>  	../../util/parse-configs.h \
> diff --git a/daxctl/lib/libdaxctl.sym b/daxctl/lib/libdaxctl.sym index
> a13e93d..190b605 100644
> --- a/daxctl/lib/libdaxctl.sym
> +++ b/daxctl/lib/libdaxctl.sym
> @@ -96,4 +96,6 @@ LIBDAXCTL_9 {
>  global:
>  	daxctl_dev_will_auto_online_memory;
>  	daxctl_dev_has_online_memory;
> +	daxctl_set_configs;
> +	daxctl_get_configs;
>  } LIBDAXCTL_8;
> --
> 2.31.1

Looks good to me.
Thank you very much.

QI

RE: [ndctl PATCH 5/7] util/parse-configs: add a key/value search helper

[qi.fuli]

> Subject: [ndctl PATCH 5/7] util/parse-configs: add a key/value search helper
> 
> Add a new config query type called CONFIG_SEARCH_SECTION, which searches
> all loaded config files based on a query criteria of: specified section name,
> specified key/value pair within that section, and can return other key/values from
> the section that matched the search criteria.
> 
> This allows for multiple named subsections, where a subsection name is of the
> type: '[section subsection]'.
> 
> Cc: QI Fuli <qi.fuli@fujitsu.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  util/parse-configs.h | 15 +++++++++++++  util/parse-configs.c | 51
> ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 66 insertions(+)
> 
> diff --git a/util/parse-configs.h b/util/parse-configs.h index 491aebb..6dcc01c
> 100644
> --- a/util/parse-configs.h
> +++ b/util/parse-configs.h
> @@ -9,6 +9,7 @@
> 
>  enum parse_conf_type {
>  	CONFIG_STRING,
> +	CONFIG_SEARCH_SECTION,
>  	CONFIG_END,
>  	MONITOR_CALLBACK,
>  };
> @@ -20,6 +21,10 @@ typedef int parse_conf_cb(const struct config *, const char
> *config_file);
> 
>  struct config {
>  	enum parse_conf_type type;
> +	const char *section;
> +	const char *search_key;
> +	const char *search_val;
> +	const char *get_key;
>  	const char *key;
>  	void *value;
>  	void *defval;
> @@ -31,6 +36,16 @@ struct config {
>  #define CONF_END() { .type = CONFIG_END }  #define CONF_STR(k,v,d) \
>  	{ .type = CONFIG_STRING, .key = (k), .value = check_vtype(v, const char
> **), .defval = (d) }
> +#define CONF_SEARCH(s, sk, sv, gk, v, d)	\
> +{						\
> +	.type = CONFIG_SEARCH_SECTION,		\
> +	.section = (s),				\
> +	.search_key = (sk),			\
> +	.search_val = (sv),			\
> +	.get_key = (gk),			\
> +	.value = check_vtype(v, const char **),	\
> +	.defval = (d)				\
> +}
>  #define CONF_MONITOR(k,f) \
>  	{ .type = MONITOR_CALLBACK, .key = (k), .callback = (f)}
> 
> diff --git a/util/parse-configs.c b/util/parse-configs.c index 72c4913..8eabe3d
> 100644
> --- a/util/parse-configs.c
> +++ b/util/parse-configs.c
> @@ -38,6 +38,54 @@ static void set_str_val(const char **value, const char *val)
>  	*value = strbuf_detach(&buf, NULL);
>  }
> 
> +static const char *search_section_kv(dictionary *d, const struct config
> +*c) {
> +	int i;
> +
> +	for (i = 0; i < iniparser_getnsec(d); i++) {
> +		const char *cur_sec_full = iniparser_getsecname(d, i);
> +		char *cur_sec = strdup(cur_sec_full);
> +		const char *search_val, *ret_val;
> +		const char *delim = " \t\n\r";
> +		char *save, *cur, *query;
> +
> +		if (!cur_sec)
> +			return NULL;
> +		if (!c->section || !c->search_key || !c->search_val
> || !c->get_key) {
> +			fprintf(stderr, "warning: malformed config query,
> skipping\n");
> +			return NULL;
> +		}
> +
> +		cur = strtok_r(cur_sec, delim, &save);
> +		if ((cur == NULL) || (strcmp(cur, c->section) != 0))
> +			goto out_sec;
> +
> +		if (asprintf(&query, "%s:%s", cur_sec_full, c->search_key) < 0)
> +			goto out_sec;
> +		search_val = iniparser_getstring(d, query, NULL);
> +		if (!search_val)
> +			goto out_query;
> +		if (strcmp(search_val, c->search_val) != 0)
> +			goto out_query;
> +
> +		/* we're now in a matching section */
> +		free(query);
> +		if (asprintf(&query, "%s:%s", cur_sec_full, c->get_key) < 0)
> +			goto out_sec;
> +		ret_val = iniparser_getstring(d, query, NULL);
> +		free(query);
> +		free(cur_sec);
> +		return ret_val;
> +
> +out_query:
> +		free(query);
> +out_sec:
> +		free(cur_sec);
> +	}
> +
> +	return NULL;
> +}
> +
>  static int parse_config_file(const char *config_file,
>  			const struct config *configs)
>  {
> @@ -54,6 +102,9 @@ static int parse_config_file(const char *config_file,
>  					iniparser_getstring(dic,
>  					configs->key, configs->defval));
>  			break;
> +		case CONFIG_SEARCH_SECTION:
> +			set_str_val((const char **)configs->value,
> +					search_section_kv(dic, configs));
>  		case MONITOR_CALLBACK:
>  		case CONFIG_END:
>  			break;
> --
> 2.31.1

Thank you very much.
This looks good to me.

QI

RE: [ndctl PATCH 7/7] daxctl: add systemd service and udev rule for auto-onlining

[qi.fuli]

> Subject: [ndctl PATCH 7/7] daxctl: add systemd service and udev rule for
> auto-onlining
> 
> Install a helper script that calls daxctl-reconfigure-device with the
> new 'check-config' option for a given device. This is meant to be called
> via a systemd service.
> 
> Install a systemd service that calls the above wrapper script with a
> daxctl device passed in to it via the environment.
> 
> Install a udev rule that is triggered for every daxctl device, and
> triggers the above oneshot systemd service.
> 
> Together, these three things work such that upon boot, whenever a daxctl
> device is found, udev triggers a device-specific systemd service called,
> for example:
> 
>   daxdev-reconfigure@-dev-dax0.0.service
> 
> This initiates a daxctl-reconfigure-device with a config lookup for the
> 'dax0.0' device. If the config has an '[auto-online <unique_id>]'
> section, it uses the information in that to set the operating mode of
> the device.
> 
> If any device is in an unexpected status, 'journalctl' can be used to
> view the reconfiguration log for that device, for example:
> 
>   journalctl --unit daxdev-reconfigure@-dev-dax0.0.service
> 
> Update the RPM spec file to include the newly added files to the RPM
> build.
> 
> Cc: QI Fuli <qi.fuli@fujitsu.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  configure.ac                       |  9 ++++++++-
>  daxctl/90-daxctl-device.rules      |  1 +
>  daxctl/Makefile.am                 | 10 ++++++++++
>  daxctl/daxdev-auto-reconfigure.sh  |  3 +++
>  daxctl/daxdev-reconfigure@.service |  8 ++++++++
>  ndctl.spec.in                      |  3 +++
>  6 files changed, 33 insertions(+), 1 deletion(-)
>  create mode 100644 daxctl/90-daxctl-device.rules
>  create mode 100755 daxctl/daxdev-auto-reconfigure.sh
>  create mode 100644 daxctl/daxdev-reconfigure@.service
> 
> diff --git a/configure.ac b/configure.ac
> index 9e1c6db..df6ab10 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -160,7 +160,7 @@ AC_CHECK_FUNCS([ \
> 
>  AC_ARG_WITH([systemd],
>  	AS_HELP_STRING([--with-systemd],
> -		[Enable systemd functionality (monitor).
> @<:@default=yes@:>@]),
> +		[Enable systemd functionality. @<:@default=yes@:>@]),
>  	[], [with_systemd=yes])
> 
>  if test "x$with_systemd" = "xyes"; then
> @@ -183,6 +183,13 @@ daxctl_modprobe_data=daxctl.conf
>  AC_SUBST([daxctl_modprobe_datadir])
>  AC_SUBST([daxctl_modprobe_data])
> 
> +AC_ARG_WITH(udevrulesdir,
> +    [AS_HELP_STRING([--with-udevrulesdir=DIR], [udev rules.d directory])],
> +    [UDEVRULESDIR="$withval"],
> +    [UDEVRULESDIR='${prefix}/lib/udev/rules.d']
> +)
> +AC_SUBST(UDEVRULESDIR)
> +
>  AC_ARG_WITH([keyutils],
>  	    AS_HELP_STRING([--with-keyutils],
>  			[Enable keyutils functionality (security).
> @<:@default=yes@:>@]), [], [with_keyutils=yes])
> diff --git a/daxctl/90-daxctl-device.rules b/daxctl/90-daxctl-device.rules
> new file mode 100644
> index 0000000..ee0670f
> --- /dev/null
> +++ b/daxctl/90-daxctl-device.rules
> @@ -0,0 +1 @@
> +ACTION=="add", SUBSYSTEM=="dax", TAG+="systemd",
> ENV{SYSTEMD_WANTS}="daxdev-reconfigure@$env{DEVNAME}.service"
> diff --git a/daxctl/Makefile.am b/daxctl/Makefile.am
> index f30c485..d53bdcf 100644
> --- a/daxctl/Makefile.am
> +++ b/daxctl/Makefile.am
> @@ -28,3 +28,13 @@ daxctl_LDADD =\
>  	$(UUID_LIBS) \
>  	$(KMOD_LIBS) \
>  	$(JSON_LIBS)
> +
> +bin_SCRIPTS = daxdev-auto-reconfigure.sh
> +CLEANFILES = $(bin_SCRIPTS)

Hi Vishal,

Thank you for the patch.
I got some warnings in my test.

$ ./autogen.sh 
daxctl/Makefile.am:33: warning: CLEANFILES multiply defined in condition TRUE ...
Makefile.am.in:2: ... 'CLEANFILES' previously defined here
daxctl/Makefile.am:1:   'Makefile.am.in' included from here

----------------------------------------------------------------
Initialized build system. For a common configuration please run:
----------------------------------------------------------------

./configure CFLAGS='-g -O2' --prefix=/usr --sysconfdir=/etc --libdir=/usr/lib64

QI

> +
> +udevrulesdir = $(UDEVRULESDIR)
> +udevrules_DATA = 90-daxctl-device.rules
> +
> +if ENABLE_SYSTEMD_UNITS
> +systemd_unit_DATA = daxdev-reconfigure@.service
> +endif
> diff --git a/daxctl/daxdev-auto-reconfigure.sh
> b/daxctl/daxdev-auto-reconfigure.sh
> new file mode 100755
> index 0000000..f6da43f
> --- /dev/null
> +++ b/daxctl/daxdev-auto-reconfigure.sh
> @@ -0,0 +1,3 @@
> +#!/bin/bash
> +
> +daxctl reconfigure-device --check-config "${1##*/}"
> diff --git a/daxctl/daxdev-reconfigure@.service
> b/daxctl/daxdev-reconfigure@.service
> new file mode 100644
> index 0000000..451fef1
> --- /dev/null
> +++ b/daxctl/daxdev-reconfigure@.service
> @@ -0,0 +1,8 @@
> +[Unit]
> +Description=Automatic daxctl device reconfiguration
> +Documentation=man:daxctl-reconfigure-device(1)
> +
> +[Service]
> +Type=forking
> +GuessMainPID=false
> +ExecStart=/bin/sh -c "exec daxdev-auto-reconfigure.sh %I"
> diff --git a/ndctl.spec.in b/ndctl.spec.in
> index 07c36ec..fd1a5ff 100644
> --- a/ndctl.spec.in
> +++ b/ndctl.spec.in
> @@ -124,8 +124,11 @@ make check
>  %defattr(-,root,root)
>  %license LICENSES/preferred/GPL-2.0 LICENSES/other/MIT
> LICENSES/other/CC0-1.0
>  %{_bindir}/daxctl
> +%{_bindir}/daxdev-auto-reconfigure.sh
>  %{_mandir}/man1/daxctl*
>  %{_datadir}/daxctl/daxctl.conf
> +%{_unitdir}/daxdev-reconfigure@.service
> +%config %{_udevrulesdir}/90-daxctl-device.rules
> 
>  %files -n LNAME
>  %defattr(-,root,root)
> --
> 2.31.1

Re: [ndctl PATCH 0/7] Policy based reconfiguration for daxctl

[Dan Williams]

On Tue, Aug 31, 2021 at 2:05 AM Vishal Verma <vishal.l.verma@intel.com> wrote:
>
> These patches add policy (config file) support to daxctl. The
> introductory user is daxctl-reconfigure-device. Sysadmins may wish to
> use daxctl devices as system-ram, but it may be cumbersome to automate
> the reconfiguration step for every device upon boot.
>
> Introduce a new option for daxctl-reconfigure-device, --check-config.
> This is at the heart of policy based reconfiguration, as it allows
> daxctl to look up reconfiguration parameters for a given device from the
> config system instead of the command line.
>
> Some systemd and udev glue then automates this for every new dax device
> that shows up, providing a way for the administrator to simply list all
> the 'system-ram' UUIDs in a config file, and not have to worry about
> anything else.
>
> An example config file can be:
>
>   # cat /etc/ndctl/daxctl.conf

Take these comments as provisional until I read through the rest, but
this is just a reaction to the proposed ini format.

>
>   [auto-online unique_identifier_foo]

I am thinking this section name should be "reconfigure-device
unique_identifier_foo" if only because resize might also be something
someone wants to do, and if other commands get config automation it
makes it clearer which config snippets apply to which command.

>   uuid = 48d8e42c-a2f0-4312-9e70-a837faafe862

I think this should be called:

"nvdimm.uuid"

...or something like that to make it clear this depends on dax devices
emitted by libnvdimm, and not those that come from "soft-reserved"
memory. It also helps distinguish if we ever get UUIDs in the HMAT
which is something I have been meaning to propose.

>   mode = system-ram

I can see this being "mode = devdax" if feature was being used to
change size or alignment.

>   online = true
>   movable = false

I wonder if these keys should be prefixed by the mode name:

system-ram.online = true
system-ram.movable = false

...so it's a bit more self documenting about which parameters are
sub-options, and delineates them from generic options like size.

> Any file under '/etc/ndctl/' can be used - all files with a '.conf' suffix
> will be considered when looking for matches.

any concern about name collisions between ndctl, daxctl, and cxl-cli
section names?

Re: [ndctl PATCH 2/7] daxctl: Documentation updates for persistent reconfiguration

[Dan Williams]

On Tue, Aug 31, 2021 at 2:05 AM Vishal Verma <vishal.l.verma@intel.com> wrote:
>
> Add a man page update describing how daxctl-reconfigure-device(1) can
> be used for persistent reconfiguration of a daxctl device using a
> config file.
>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  .../daxctl/daxctl-reconfigure-device.txt      | 67 +++++++++++++++++++
>  1 file changed, 67 insertions(+)
>
> diff --git a/Documentation/daxctl/daxctl-reconfigure-device.txt b/Documentation/daxctl/daxctl-reconfigure-device.txt
> index f112b3c..1e2d380 100644
> --- a/Documentation/daxctl/daxctl-reconfigure-device.txt
> +++ b/Documentation/daxctl/daxctl-reconfigure-device.txt
> @@ -162,6 +162,15 @@ include::region-option.txt[]
>         brought online automatically and immediately with the 'online_movable'
>         policy. Use this option to disable the automatic onlining behavior.
>
> +-C::
> +--check-config::
> +       Get reconfiguration parameters from the global daxctl config file.
> +       This is typically used when daxctl-reconfigure-device is called from
> +       a systemd-udevd device unit file. The reconfiguration proceeds only
> +       if the UUID of the dax device passed in on the command line matches
> +       a UUID listed in the auto-online section of the config. See the
> +       'PERSISTENT RECONFIGURATION' section for more details.

There's going to be other match parameters in the future, so this can
probably say something like:

"The reconfiguration proceeds only if the match parameters in a
'reconfigure-device' section of the config match dax device specified
on the command line"

> +
>  include::movable-options.txt[]
>
>  -f::
> @@ -183,6 +192,64 @@ include::human-option.txt[]
>
>  include::verbose-option.txt[]
>
> +PERSISTENT RECONFIGURATION
> +--------------------------
> +
> +The 'mode' of a daxctl device is not persistent across reboots by default. This
> +is because the device itself may hold any metadata that hints at what mode it

s/may hold any/does not hold/

> +was set to, or is intended to be used in. The default mode for such a device

s/in//

> +is 'devdax', and on reboot, that is the mode devices appear in by default.

s/, that is the mode devices appear in by default//

i.e. redundant.

> +
> +The administrator may desire to configure the system in a way that certain

How about:

"The administrator may set policy such that certain dax devices are
always reconfigured into a target configuration every boot."

> +dax devices are always reconfigured into a certain mode every time on boot.
> +This is accomplished via a daxctl config file located at [location TBD].
> +
> +The config file may have multiple sections influencing different aspects of
> +daxctl operation. The section of interest for persistent reconfiguration is
> +'auto-online'. The format of this is as follows:
> +
> +----
> +[auto-online <subsection_name>]
> +uuid = <namespace uuid>
> +mode = <desired reconfiguration mode> (default: system-ram)
> +online = <true|false> (default: true)
> +movable = <true|false> (default: true)
> +----
> +
> +Here is an example of a config snippet for managing three devdax namespaces,
> +one is left in devdax mode, the second is changed to system-ram mode with
> +default options (online, movable), and the third is set to system-ram mode,
> +the memory is onlined, but not movable.
> +
> +Note that the 'subsection name' can be arbitrary, and is only used to
> +identify a specific config section. It does not have to match the 'device
> +name' (e.g. 'dax0.0' etc).
> +
> +----
> +[auto-online dax0]
> +uuid = ed93e918-e165-49d8-921d-383d7b9660c5
> +mode = devdax
> +
> +[auto-online dax1]
> +uuid = f36d02ff-1d9f-4fb9-a5b9-8ceb10a00fe3
> +mode = system-ram
> +
> +[auto-online dax2]
> +uuid = f36d02ff-1d9f-4fb9-a5b9-8ceb10a00fe3
> +mode = system-ram
> +online = true
> +movable = false

One of the tasks I envisioned with persistent configurations was
recalling resize and create device operations. Not saying that needs
to be included now, but I can assume that these reconfiguration steps
are performed in order... Hmm, as I ask that I realize it may depend
on device arrival. Ok, assuming the devices have all arrived by the
time this runs is there a guarantee that these are processed in order?

> +----
> +
> +The following example can be used to create a devdax mode namespace, and
> +simultaneously add the newly created namespace to the config file for
> +system-ram conversion.
> +
> +----
> +ndctl create-namespace --mode=devdax | \
> +       jq -r "\"[auto-online $(uuidgen)]\", \"uuid = \(.uuid)\", \"mode = system-ram\"" >> $config_path
> +----
> +
>  include::../copyright.txt[]
>
>  SEE ALSO
> --
> 2.31.1
>

Re: [ndctl PATCH 3/7] util/parse-config: refactor filter_conf_files into util/

[Dan Williams]

On Tue, Aug 31, 2021 at 2:05 AM Vishal Verma <vishal.l.verma@intel.com> wrote:
>
> Move filter_conf() into util/parse-configs.c as filter_conf_files() so
> that it can be reused by the config parser in daxctl.
>
> Cc: QI Fuli <qi.fuli@fujitsu.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  ndctl/lib/libndctl.c   | 19 ++-----------------
>  util/parse-configs.h   |  4 ++++
>  util/parse-configs.c   | 16 ++++++++++++++++
>  daxctl/lib/Makefile.am |  2 ++
>  ndctl/lib/Makefile.am  |  2 ++
>  5 files changed, 26 insertions(+), 17 deletions(-)
>
> diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
> index db2e38b..a01ac80 100644
> --- a/ndctl/lib/libndctl.c
> +++ b/ndctl/lib/libndctl.c
> @@ -25,6 +25,7 @@
>  #include <util/size.h>
>  #include <util/sysfs.h>
>  #include <util/strbuf.h>
> +#include <util/parse-configs.h>
>  #include <ndctl/libndctl.h>
>  #include <ndctl/namespace.h>
>  #include <daxctl/libdaxctl.h>
> @@ -266,22 +267,6 @@ NDCTL_EXPORT void ndctl_set_userdata(struct ndctl_ctx *ctx, void *userdata)
>         ctx->userdata = userdata;
>  }
>
> -static int filter_conf(const struct dirent *dir)
> -{
> -       if (!dir)
> -               return 0;
> -
> -       if (dir->d_type == DT_REG) {
> -               const char *ext = strrchr(dir->d_name, '.');
> -               if ((!ext) || (ext == dir->d_name))
> -                       return 0;
> -               if (strcmp(ext, ".conf") == 0)
> -                       return 1;
> -       }
> -
> -       return 0;
> -}
> -
>  NDCTL_EXPORT void ndctl_set_configs(struct ndctl_ctx **ctx, char *conf_dir)
>  {
>         struct dirent **namelist;
> @@ -291,7 +276,7 @@ NDCTL_EXPORT void ndctl_set_configs(struct ndctl_ctx **ctx, char *conf_dir)
>         if ((!ctx) || (!conf_dir))
>                 return;
>
> -       rc = scandir(conf_dir, &namelist, filter_conf, alphasort);
> +       rc = scandir(conf_dir, &namelist, filter_conf_files, alphasort);
>         if (rc == -1) {
>                 perror("scandir");
>                 return;
> diff --git a/util/parse-configs.h b/util/parse-configs.h
> index f70f58f..491aebb 100644
> --- a/util/parse-configs.h
> +++ b/util/parse-configs.h
> @@ -1,8 +1,10 @@
>  // SPDX-License-Identifier: GPL-2.0
>  // Copyright (C) 2021, FUJITSU LIMITED. ALL rights reserved.
>
> +#include <dirent.h>
>  #include <stdbool.h>
>  #include <stdint.h>
> +#include <string.h>
>  #include <util/util.h>
>
>  enum parse_conf_type {
> @@ -11,6 +13,8 @@ enum parse_conf_type {
>         MONITOR_CALLBACK,
>  };
>
> +int filter_conf_files(const struct dirent *dir);
> +
>  struct config;
>  typedef int parse_conf_cb(const struct config *, const char *config_file);
>
> diff --git a/util/parse-configs.c b/util/parse-configs.c
> index 44dcff4..72c4913 100644
> --- a/util/parse-configs.c
> +++ b/util/parse-configs.c
> @@ -6,6 +6,22 @@
>  #include <util/strbuf.h>
>  #include <util/iniparser.h>
>
> +int filter_conf_files(const struct dirent *dir)
> +{
> +       if (!dir)
> +               return 0;
> +
> +       if (dir->d_type == DT_REG) {
> +               const char *ext = strrchr(dir->d_name, '.');

Darn, I guess clang-format does not know how to do line-break after
declarations, but... line break after declarations please. Otherwise
it looks good.

> +               if ((!ext) || (ext == dir->d_name))
> +                       return 0;
> +               if (strcmp(ext, ".conf") == 0)
> +                       return 1;
> +       }
> +
> +       return 0;
> +}
> +
>  static void set_str_val(const char **value, const char *val)
>  {
>         struct strbuf buf = STRBUF_INIT;
> diff --git a/daxctl/lib/Makefile.am b/daxctl/lib/Makefile.am
> index 25efd83..db2351e 100644
> --- a/daxctl/lib/Makefile.am
> +++ b/daxctl/lib/Makefile.am
> @@ -15,6 +15,8 @@ libdaxctl_la_SOURCES =\
>         ../../util/sysfs.h \
>         ../../util/log.c \
>         ../../util/log.h \
> +       ../../util/parse-configs.h \
> +       ../../util/parse-configs.c \
>         libdaxctl.c
>
>  libdaxctl_la_LIBADD =\
> diff --git a/ndctl/lib/Makefile.am b/ndctl/lib/Makefile.am
> index f741c44..8020eb4 100644
> --- a/ndctl/lib/Makefile.am
> +++ b/ndctl/lib/Makefile.am
> @@ -19,6 +19,8 @@ libndctl_la_SOURCES =\
>         ../../util/wrapper.c \
>         ../../util/usage.c \
>         ../../util/fletcher.h \
> +       ../../util/parse-configs.h \
> +       ../../util/parse-configs.c \
>         dimm.c \
>         inject.c \
>         nfit.c \
> --
> 2.31.1
>

Re: [ndctl PATCH 4/7] daxctl: add basic config parsing support

[Dan Williams]

On Tue, Aug 31, 2021 at 2:05 AM Vishal Verma <vishal.l.verma@intel.com> wrote:
>
> Add support similar to ndctl and libndctl for parsing config files. This
> allows storing a config file path/list in the daxctl_ctx, and adds APIs
> for setting and retrieving it.
>
> Cc: QI Fuli <qi.fuli@fujitsu.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  daxctl/lib/libdaxctl.c   | 37 +++++++++++++++++++++++++++++++++++++
>  daxctl/libdaxctl.h       |  2 ++
>  daxctl/Makefile.am       |  1 +
>  daxctl/lib/Makefile.am   |  4 ++++
>  daxctl/lib/libdaxctl.sym |  2 ++
>  5 files changed, 46 insertions(+)
>
> diff --git a/daxctl/lib/libdaxctl.c b/daxctl/lib/libdaxctl.c
> index 860bd9c..659d2fe 100644
> --- a/daxctl/lib/libdaxctl.c
> +++ b/daxctl/lib/libdaxctl.c
> @@ -17,6 +17,8 @@
>  #include <util/log.h>
>  #include <util/sysfs.h>
>  #include <util/iomem.h>
> +#include <util/strbuf.h>
> +#include <util/parse-configs.h>
>  #include <daxctl/libdaxctl.h>
>  #include "libdaxctl-private.h"
>
> @@ -37,6 +39,7 @@ struct daxctl_ctx {
>         struct log_ctx ctx;
>         int refcount;
>         void *userdata;
> +       const char *configs;
>         int regions_init;
>         struct list_head regions;
>         struct kmod_ctx *kmod_ctx;
> @@ -68,6 +71,40 @@ DAXCTL_EXPORT void daxctl_set_userdata(struct daxctl_ctx *ctx, void *userdata)
>         ctx->userdata = userdata;
>  }
>
> +DAXCTL_EXPORT void daxctl_set_configs(struct daxctl_ctx **ctx, char *conf_dir)
> +{
> +       struct dirent **namelist;
> +       struct strbuf value = STRBUF_INIT;
> +       int rc;
> +
> +       if ((!ctx) || (!conf_dir))
> +               return;
> +
> +       rc = scandir(conf_dir, &namelist, filter_conf_files, alphasort);
> +       if (rc == -1) {
> +               perror("scandir");
> +               return;
> +       }
> +
> +       while (rc--) {
> +               if (value.len)
> +                       strbuf_addstr(&value, " ");
> +               strbuf_addstr(&value, conf_dir);
> +               strbuf_addstr(&value, "/");
> +               strbuf_addstr(&value, namelist[rc]->d_name);
> +               free(namelist[rc]);
> +       }
> +       (*ctx)->configs = strbuf_detach(&value, NULL);
> +       free(namelist);
> +}
> +
> +DAXCTL_EXPORT const char *daxctl_get_configs(struct daxctl_ctx *ctx)
> +{
> +       if (ctx == NULL)
> +               return NULL;
> +       return ctx->configs;
> +}
> +
>  /**
>   * daxctl_new - instantiate a new library context
>   * @ctx: context to establish
> diff --git a/daxctl/libdaxctl.h b/daxctl/libdaxctl.h
> index 683ae9c..9388f85 100644
> --- a/daxctl/libdaxctl.h
> +++ b/daxctl/libdaxctl.h
> @@ -28,6 +28,8 @@ int daxctl_get_log_priority(struct daxctl_ctx *ctx);
>  void daxctl_set_log_priority(struct daxctl_ctx *ctx, int priority);
>  void daxctl_set_userdata(struct daxctl_ctx *ctx, void *userdata);
>  void *daxctl_get_userdata(struct daxctl_ctx *ctx);
> +void daxctl_set_configs(struct daxctl_ctx **ctx, char *conf_dir);
> +const char *daxctl_get_configs(struct daxctl_ctx *ctx);
>
>  struct daxctl_region;
>  struct daxctl_region *daxctl_new_region(struct daxctl_ctx *ctx, int id,
> diff --git a/daxctl/Makefile.am b/daxctl/Makefile.am
> index 9b1313a..a9845a0 100644
> --- a/daxctl/Makefile.am
> +++ b/daxctl/Makefile.am
> @@ -10,6 +10,7 @@ config.h: $(srcdir)/Makefile.am
>                 "$(daxctl_modprobe_datadir)/$(daxctl_modprobe_data)"' >>$@ && \
>         echo '#define DAXCTL_MODPROBE_INSTALL \
>                 "$(sysconfdir)/modprobe.d/$(daxctl_modprobe_data)"' >>$@
> +       $(AM_V_GEN) echo '#define DAXCTL_CONF_DIR  "$(ndctl_confdir)"' >>$@

This gets back to my namespace question about collisions between
daxctl, ndctl, and cxl-cli conf snippets. I think they should each get
their own directory in /etc, then we don't need to encode any prefixes
into section names. What do you think?

Re: [ndctl PATCH 5/7] util/parse-configs: add a key/value search helper

[Dan Williams]

On Tue, Aug 31, 2021 at 2:05 AM Vishal Verma <vishal.l.verma@intel.com> wrote:
>
> Add a new config query type called CONFIG_SEARCH_SECTION, which searches
> all loaded config files based on a query criteria of: specified section
> name, specified key/value pair within that section, and can return other
> key/values from the section that matched the search criteria.
>
> This allows for multiple named subsections, where a subsection name is
> of the type: '[section subsection]'.

Presumably in the future this could be used to search by subsection as
well, but for now daxctl does not need that and the subsection is
essentially a comment?

Perhaps that should be called out in a sample / comment only
daxctl.conf file that gets installed by default. Where it clarifies
that everything after the first whitespace in a section name is
treated as a subsection.

This looks good to me.

Re: [ndctl PATCH 6/7] daxctl/device.c: add an option for getting params from a config file

[Dan Williams]

On Tue, Aug 31, 2021 at 2:05 AM Vishal Verma <vishal.l.verma@intel.com> wrote:
>
> Add a new option to daxctl-reconfigure-device that allows it to
> comprehend the new global config system in ndctl/daxctl. With this, the
> reconfigure-device command can query the config to match a specific
> device UUID, and operate using the parameters supplied in that INI
> section.
>
> This is in preparation to make daxctl device reconfiguration (usually
> as system-ram) policy based, so that reconfiguration can happen
> automatically on boot.
>
> Cc: QI Fuli <qi.fuli@fujitsu.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  daxctl/daxctl.c    |   1 +
>  daxctl/device.c    | 141 ++++++++++++++++++++++++++++++++++++++++++++-
>  daxctl/Makefile.am |   1 +
>  3 files changed, 142 insertions(+), 1 deletion(-)
>
> diff --git a/daxctl/daxctl.c b/daxctl/daxctl.c
> index 928814c..dc7ac5f 100644
> --- a/daxctl/daxctl.c
> +++ b/daxctl/daxctl.c
> @@ -95,6 +95,7 @@ int main(int argc, const char **argv)
>         rc = daxctl_new(&ctx);
>         if (rc)
>                 goto out;
> +       daxctl_set_configs(&ctx, DAXCTL_CONF_DIR);

Now that I see how this is used, I think this wants to be called:

daxctl_ctx_add_configs()

...because the operation is populating the context with all the
configuration fragments found in the given directory.

I would also have an option to specify an override for the config
directory, maybe that could be an environment variable?

>         main_handle_internal_command(argc, argv, ctx, commands,
>                         ARRAY_SIZE(commands), PROG_DAXCTL);
>         daxctl_unref(ctx);
> diff --git a/daxctl/device.c b/daxctl/device.c
> index a427b7d..99a4548 100644
> --- a/daxctl/device.c
> +++ b/daxctl/device.c
> @@ -14,8 +14,10 @@
>  #include <util/filter.h>
>  #include <json-c/json.h>
>  #include <json-c/json_util.h>
> +#include <ndctl/libndctl.h>
>  #include <daxctl/libdaxctl.h>
>  #include <util/parse-options.h>
> +#include <util/parse-configs.h>
>  #include <ccan/array_size/array_size.h>
>
>  static struct {
> @@ -25,6 +27,7 @@ static struct {
>         const char *size;
>         const char *align;
>         const char *input;
> +       bool check_config;
>         bool no_online;
>         bool no_movable;
>         bool force;
> @@ -75,7 +78,9 @@ OPT_STRING('m', "mode", &param.mode, "mode", "mode to switch the device to"), \
>  OPT_BOOLEAN('N', "no-online", &param.no_online, \
>         "don't auto-online memory sections"), \
>  OPT_BOOLEAN('f', "force", &param.force, \
> -               "attempt to offline memory sections before reconfiguration")
> +               "attempt to offline memory sections before reconfiguration"), \
> +OPT_BOOLEAN('C', "check-config", &param.check_config, \
> +               "use config files to determine parameters for the operation")
>
>  #define CREATE_OPTIONS() \
>  OPT_STRING('s', "size", &param.size, "size", "size to switch the device to"), \
> @@ -218,6 +223,130 @@ err:
>         return rc;
>  }
>
> +static int conf_string_to_bool(const char *str)
> +{
> +       if (!str)
> +               return INT_MAX;
> +       if (strncmp(str, "t", 1) == 0 ||
> +                       strncmp(str, "T", 1) == 0 ||
> +                       strncmp(str, "y", 1) == 0 ||
> +                       strncmp(str, "Y", 1) == 0 ||
> +                       strncmp(str, "1", 1) == 0)
> +               return true;
> +       if (strncmp(str, "f", 1) == 0 ||
> +                       strncmp(str, "F", 1) == 0 ||
> +                       strncmp(str, "n", 1) == 0 ||
> +                       strncmp(str, "N", 1) == 0 ||
> +                       strncmp(str, "0", 1) == 0)
> +               return false;

Doesn't iniparser_getboolean() already do this conversion for you?

> +       return INT_MAX;
> +}
> +
> +#define conf_assign_inverted_bool(p, conf_var) \
> +do { \
> +       if (conf_string_to_bool(conf_var) != INT_MAX) \
> +               param.p = !conf_string_to_bool(conf_var); \
> +} while(0)
> +
> +static int parse_config_reconfig_set_params(struct daxctl_ctx *ctx, const char *device,
> +                                           const char *uuid)
> +{
> +       const char *conf_online = NULL, *conf_movable = NULL;
> +       const struct config configs[] = {
> +               CONF_SEARCH("auto-online", "uuid", uuid, "mode", &param.mode, NULL),
> +               CONF_SEARCH("auto-online", "uuid", uuid, "online", &conf_online, NULL),
> +               CONF_SEARCH("auto-online", "uuid", uuid, "movable", &conf_movable, NULL),
> +               CONF_END(),
> +       };
> +       const char *prefix = "./";
> +       int rc;
> +
> +       rc = parse_configs_prefix(daxctl_get_configs(ctx), prefix, configs);
> +       if (rc < 0)
> +               return rc;
> +
> +       conf_assign_inverted_bool(no_online, conf_online);
> +       conf_assign_inverted_bool(no_movable, conf_movable);
> +
> +       return 0;
> +}
> +
> +static bool daxctl_ndns_has_device(struct ndctl_namespace *ndns,
> +                                   const char *device)
> +{
> +       struct daxctl_region *dax_region;
> +       struct ndctl_dax *dax;
> +
> +       dax = ndctl_namespace_get_dax(ndns);
> +       if (!dax)
> +               return false;
> +
> +       dax_region = ndctl_dax_get_daxctl_region(dax);
> +       if (dax_region) {
> +               struct daxctl_dev *dev;
> +
> +               dev = daxctl_dev_get_first(dax_region);
> +               if (dev) {
> +                       if (strcmp(daxctl_dev_get_devname(dev), device) == 0)
> +                               return true;
> +               }
> +       }
> +       return false;
> +}
> +
> +static int parse_config_reconfig(struct daxctl_ctx *ctx, const char *device)
> +{
> +       struct ndctl_namespace *ndns;
> +       struct ndctl_ctx *ndctl_ctx;
> +       struct ndctl_region *region;
> +       struct ndctl_bus *bus;
> +       struct ndctl_dax *dax;
> +       int rc, found = 0;
> +       char uuid_buf[40];
> +       uuid_t uuid;
> +
> +       if (strcmp(device, "all") == 0)
> +               return 0;
> +
> +       rc = ndctl_new(&ndctl_ctx);
> +       if (rc < 0)
> +               return rc;
> +
> +        ndctl_bus_foreach(ndctl_ctx, bus) {
> +               ndctl_region_foreach(bus, region) {
> +                       ndctl_namespace_foreach(region, ndns) {
> +                               if (daxctl_ndns_has_device(ndns, device)) {
> +                                       dax = ndctl_namespace_get_dax(ndns);
> +                                       if (!dax)
> +                                               continue;
> +                                       ndctl_dax_get_uuid(dax, uuid);
> +                                       found = 1;
> +                               }
> +                       }
> +               }
> +       }
> +
> +       if (!found) {
> +               fprintf(stderr, "no UUID match for %s found in config files\n",
> +                       device);
> +               return 0;
> +       }
> +
> +       uuid_unparse(uuid, uuid_buf);
> +       return parse_config_reconfig_set_params(ctx, device, uuid_buf);
> +}
> +
> +static int parse_device_config(struct daxctl_ctx *ctx, const char *device,
> +                              enum device_action action)
> +{
> +       switch (action) {
> +       case ACTION_RECONFIG:
> +               return parse_config_reconfig(ctx, device);
> +       default:
> +               return 0;
> +       }
> +}
> +
>  static const char *parse_device_options(int argc, const char **argv,
>                 enum device_action action, const struct option *options,
>                 const char *usage, struct daxctl_ctx *ctx)
> @@ -279,6 +408,16 @@ static const char *parse_device_options(int argc, const char **argv,
>         if (param.human)
>                 flags |= UTIL_JSON_HUMAN;
>
> +       /* Scan config file(s) for options. This sets param.foo accordingly */
> +       if (param.check_config) {
> +               rc = parse_device_config(ctx, argv[0], action);

What happens if someone does:

daxctl reconfigure-device -C --no-online /dev/dax0.0

...and it matches an entry in the configuration file that has
"system-ram.online = true".

My expectation is that precedence ordering is:

built-in defaults
configuration file settings
command line options

Where later entries in that list override settings from the previous
entry. That said, I don't see an easy way to achieve this with parse
options, so might need to fail if anything but -C is specified as an
option until we can fix that conflict.

> +               if (rc) {
> +                       fprintf(stderr, "error parsing config file: %s\n",
> +                               strerror(-rc));
> +                       return NULL;
> +               }
> +       }
> +
>         /* Handle action-specific options */
>         switch (action) {
>         case ACTION_RECONFIG:
> diff --git a/daxctl/Makefile.am b/daxctl/Makefile.am
> index a9845a0..f30c485 100644
> --- a/daxctl/Makefile.am
> +++ b/daxctl/Makefile.am
> @@ -23,6 +23,7 @@ daxctl_SOURCES =\
>
>  daxctl_LDADD =\
>         lib/libdaxctl.la \
> +       ../ndctl/lib/libndctl.la \
>         ../libutil.a \
>         $(UUID_LIBS) \
>         $(KMOD_LIBS) \
> --
> 2.31.1
>

Re: [ndctl PATCH 7/7] daxctl: add systemd service and udev rule for auto-onlining

[Dan Williams]

On Tue, Aug 31, 2021 at 2:05 AM Vishal Verma <vishal.l.verma@intel.com> wrote:
>
> Install a helper script that calls daxctl-reconfigure-device with the
> new 'check-config' option for a given device. This is meant to be called
> via a systemd service.
>
> Install a systemd service that calls the above wrapper script with a
> daxctl device passed in to it via the environment.
>
> Install a udev rule that is triggered for every daxctl device, and
> triggers the above oneshot systemd service.
>
> Together, these three things work such that upon boot, whenever a daxctl
> device is found, udev triggers a device-specific systemd service called,
> for example:
>
>   daxdev-reconfigure@-dev-dax0.0.service

I'm thinking the service would be called daxdev-add, because it is
servicing KOBJ_ADD events, or is the convention to name the service
after what it does vs what it services?

Also, I'm curious why would "dax0.0" be in the service name, shouldn't
this be scanning all dax devices and internally matching based on the
config file?

>
> This initiates a daxctl-reconfigure-device with a config lookup for the
> 'dax0.0' device. If the config has an '[auto-online <unique_id>]'
> section, it uses the information in that to set the operating mode of
> the device.
>
> If any device is in an unexpected status, 'journalctl' can be used to
> view the reconfiguration log for that device, for example:
>
>   journalctl --unit daxdev-reconfigure@-dev-dax0.0.service

There will be a log per-device, or only if there is a service per
device? My assumption was that this service is firing off for all
devices so you would need to filter the log by the device-name if you
know it... or maybe I'm misunderstanding how this udev service works.

>
> Update the RPM spec file to include the newly added files to the RPM
> build.
>
> Cc: QI Fuli <qi.fuli@fujitsu.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  configure.ac                       |  9 ++++++++-
>  daxctl/90-daxctl-device.rules      |  1 +
>  daxctl/Makefile.am                 | 10 ++++++++++
>  daxctl/daxdev-auto-reconfigure.sh  |  3 +++
>  daxctl/daxdev-reconfigure@.service |  8 ++++++++
>  ndctl.spec.in                      |  3 +++
>  6 files changed, 33 insertions(+), 1 deletion(-)
>  create mode 100644 daxctl/90-daxctl-device.rules
>  create mode 100755 daxctl/daxdev-auto-reconfigure.sh
>  create mode 100644 daxctl/daxdev-reconfigure@.service
>
> diff --git a/configure.ac b/configure.ac
> index 9e1c6db..df6ab10 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -160,7 +160,7 @@ AC_CHECK_FUNCS([ \
>
>  AC_ARG_WITH([systemd],
>         AS_HELP_STRING([--with-systemd],
> -               [Enable systemd functionality (monitor). @<:@default=yes@:>@]),
> +               [Enable systemd functionality. @<:@default=yes@:>@]),
>         [], [with_systemd=yes])
>
>  if test "x$with_systemd" = "xyes"; then
> @@ -183,6 +183,13 @@ daxctl_modprobe_data=daxctl.conf
>  AC_SUBST([daxctl_modprobe_datadir])
>  AC_SUBST([daxctl_modprobe_data])
>
> +AC_ARG_WITH(udevrulesdir,
> +    [AS_HELP_STRING([--with-udevrulesdir=DIR], [udev rules.d directory])],
> +    [UDEVRULESDIR="$withval"],
> +    [UDEVRULESDIR='${prefix}/lib/udev/rules.d']
> +)
> +AC_SUBST(UDEVRULESDIR)
> +
>  AC_ARG_WITH([keyutils],
>             AS_HELP_STRING([--with-keyutils],
>                         [Enable keyutils functionality (security).  @<:@default=yes@:>@]), [], [with_keyutils=yes])
> diff --git a/daxctl/90-daxctl-device.rules b/daxctl/90-daxctl-device.rules
> new file mode 100644
> index 0000000..ee0670f
> --- /dev/null
> +++ b/daxctl/90-daxctl-device.rules
> @@ -0,0 +1 @@
> +ACTION=="add", SUBSYSTEM=="dax", TAG+="systemd", ENV{SYSTEMD_WANTS}="daxdev-reconfigure@$env{DEVNAME}.service"
> diff --git a/daxctl/Makefile.am b/daxctl/Makefile.am
> index f30c485..d53bdcf 100644
> --- a/daxctl/Makefile.am
> +++ b/daxctl/Makefile.am
> @@ -28,3 +28,13 @@ daxctl_LDADD =\
>         $(UUID_LIBS) \
>         $(KMOD_LIBS) \
>         $(JSON_LIBS)
> +
> +bin_SCRIPTS = daxdev-auto-reconfigure.sh
> +CLEANFILES = $(bin_SCRIPTS)
> +
> +udevrulesdir = $(UDEVRULESDIR)
> +udevrules_DATA = 90-daxctl-device.rules
> +
> +if ENABLE_SYSTEMD_UNITS
> +systemd_unit_DATA = daxdev-reconfigure@.service
> +endif
> diff --git a/daxctl/daxdev-auto-reconfigure.sh b/daxctl/daxdev-auto-reconfigure.sh
> new file mode 100755
> index 0000000..f6da43f
> --- /dev/null
> +++ b/daxctl/daxdev-auto-reconfigure.sh
> @@ -0,0 +1,3 @@
> +#!/bin/bash
> +
> +daxctl reconfigure-device --check-config "${1##*/}"
> diff --git a/daxctl/daxdev-reconfigure@.service b/daxctl/daxdev-reconfigure@.service
> new file mode 100644
> index 0000000..451fef1
> --- /dev/null
> +++ b/daxctl/daxdev-reconfigure@.service
> @@ -0,0 +1,8 @@
> +[Unit]
> +Description=Automatic daxctl device reconfiguration
> +Documentation=man:daxctl-reconfigure-device(1)
> +
> +[Service]
> +Type=forking
> +GuessMainPID=false
> +ExecStart=/bin/sh -c "exec daxdev-auto-reconfigure.sh %I"

Why is the daxdev-auto-reconfigure.sh indirection needed? Can this not be:

ExecStart=daxctl reconfigure-device -C %I

...if the format of %l is the issue I think it would be good for
reconfigure-device to be tolerant of this format.

Re: [ndctl PATCH 2/7] daxctl: Documentation updates for persistent reconfiguration

[Verma, Vishal L]

On Thu, 2021-09-16 at 15:47 -0700, Dan Williams wrote:
> On Tue, Aug 31, 2021 at 2:05 AM Vishal Verma <vishal.l.verma@intel.com> wrote:
> > 
> > Add a man page update describing how daxctl-reconfigure-device(1) can
> > be used for persistent reconfiguration of a daxctl device using a
> > config file.
> > 
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> > ---
> >  .../daxctl/daxctl-reconfigure-device.txt      | 67 +++++++++++++++++++
> >  1 file changed, 67 insertions(+)
> > 

[snip]
(I've made all the other documentation changes suggested).

> > 
> > +
> > +[auto-online dax2]
> > +uuid = f36d02ff-1d9f-4fb9-a5b9-8ceb10a00fe3
> > +mode = system-ram
> > +online = true
> > +movable = false
> 
> One of the tasks I envisioned with persistent configurations was
> recalling resize and create device operations. Not saying that needs
> to be included now, but I can assume that these reconfiguration steps
> are performed in order... Hmm, as I ask that I realize it may depend
> on device arrival. Ok, assuming the devices have all arrived by the
> time this runs is there a guarantee that these are processed in order?

Hm, I don't quite follow what you mean by processing in order. The
logic here is that there are two 'classes' of config items in this
section - 'identification' and 'action' The uuid is the only
identification item - it is used to match the right device. The others
- 'action' items, dictate what will be done to that device. The order
here doesn't (shouldn't?) matter as these just set the param.foo fields
in daxctl/device.c, and let do_reconfig() run wild with them, just as
if the params were specified on the command line.

Re: [ndctl PATCH 4/7] daxctl: add basic config parsing support

[Verma, Vishal L]

On Thu, 2021-09-16 at 15:58 -0700, Dan Williams wrote:
> On Tue, Aug 31, 2021 at 2:05 AM Vishal Verma <vishal.l.verma@intel.com> wrote:
> > 
> > Add support similar to ndctl and libndctl for parsing config files. This
> > allows storing a config file path/list in the daxctl_ctx, and adds APIs
> > for setting and retrieving it.
> > 
> > Cc: QI Fuli <qi.fuli@fujitsu.com>
> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> > ---
> >  daxctl/lib/libdaxctl.c   | 37 +++++++++++++++++++++++++++++++++++++
> >  daxctl/libdaxctl.h       |  2 ++
> >  daxctl/Makefile.am       |  1 +
> >  daxctl/lib/Makefile.am   |  4 ++++
> >  daxctl/lib/libdaxctl.sym |  2 ++
> >  5 files changed, 46 insertions(+)
> > 
[snip]
> > 
> > diff --git a/daxctl/Makefile.am b/daxctl/Makefile.am
> > index 9b1313a..a9845a0 100644
> > --- a/daxctl/Makefile.am
> > +++ b/daxctl/Makefile.am
> > @@ -10,6 +10,7 @@ config.h: $(srcdir)/Makefile.am
> >                 "$(daxctl_modprobe_datadir)/$(daxctl_modprobe_data)"' >>$@ && \
> >         echo '#define DAXCTL_MODPROBE_INSTALL \
> >                 "$(sysconfdir)/modprobe.d/$(daxctl_modprobe_data)"' >>$@
> > +       $(AM_V_GEN) echo '#define DAXCTL_CONF_DIR  "$(ndctl_confdir)"' >>$@
> 
> This gets back to my namespace question about collisions between
> daxctl, ndctl, and cxl-cli conf snippets. I think they should each get
> their own directory in /etc, then we don't need to encode any prefixes
> into section names. What do you think?

Yep splitting config directories sounds cleaner - I'll change this.

Re: [ndctl PATCH 5/7] util/parse-configs: add a key/value search helper

[Verma, Vishal L]

On Thu, 2021-09-16 at 16:54 -0700, Dan Williams wrote:
> On Tue, Aug 31, 2021 at 2:05 AM Vishal Verma <vishal.l.verma@intel.com> wrote:
> > 
> > Add a new config query type called CONFIG_SEARCH_SECTION, which searches
> > all loaded config files based on a query criteria of: specified section
> > name, specified key/value pair within that section, and can return other
> > key/values from the section that matched the search criteria.
> > 
> > This allows for multiple named subsections, where a subsection name is
> > of the type: '[section subsection]'.
> 
> Presumably in the future this could be used to search by subsection as
> well, but for now daxctl does not need that and the subsection is
> essentially a comment?

Correct.

> 
> Perhaps that should be called out in a sample / comment only
> daxctl.conf file that gets installed by default. Where it clarifies
> that everything after the first whitespace in a section name is
> treated as a subsection.

Hm the man page does say:

   Note that the 'subsection name' can be arbitrary, and is only used
   to identify a specific config section. It does not have to match the
   'device name' (e.g. 'dax0.0' etc).
   
I didn't include a sample/comment-only config file - I figured the man
page snippets should be sufficient for anyone who wants to write one - but
I can add something if it's useful.

> 
> This looks good to me.

Re: [ndctl PATCH 7/7] daxctl: add systemd service and udev rule for auto-onlining

[Verma, Vishal L]

On Fri, 2021-09-17 at 11:10 -0700, Dan Williams wrote:
> On Tue, Aug 31, 2021 at 2:05 AM Vishal Verma <vishal.l.verma@intel.com> wrote:
> > 
> > Install a helper script that calls daxctl-reconfigure-device with the
> > new 'check-config' option for a given device. This is meant to be called
> > via a systemd service.
> > 
> > Install a systemd service that calls the above wrapper script with a
> > daxctl device passed in to it via the environment.
> > 
> > Install a udev rule that is triggered for every daxctl device, and
> > triggers the above oneshot systemd service.
> > 
> > Together, these three things work such that upon boot, whenever a daxctl
> > device is found, udev triggers a device-specific systemd service called,
> > for example:
> > 
> >   daxdev-reconfigure@-dev-dax0.0.service
> 
> I'm thinking the service would be called daxdev-add, because it is
> servicing KOBJ_ADD events, or is the convention to name the service
> after what it does vs what it services?
> 
> Also, I'm curious why would "dax0.0" be in the service name, shouldn't
> this be scanning all dax devices and internally matching based on the
> config file?
> 
> > 
> > This initiates a daxctl-reconfigure-device with a config lookup for the
> > 'dax0.0' device. If the config has an '[auto-online <unique_id>]'
> > section, it uses the information in that to set the operating mode of
> > the device.
> > 
> > If any device is in an unexpected status, 'journalctl' can be used to
> > view the reconfiguration log for that device, for example:
> > 
> >   journalctl --unit daxdev-reconfigure@-dev-dax0.0.service
> 
> There will be a log per-device, or only if there is a service per
> device? My assumption was that this service is firing off for all
> devices so you would need to filter the log by the device-name if you
> know it... or maybe I'm misunderstanding how this udev service works.
> 
> > 
> > Update the RPM spec file to include the newly added files to the RPM
> > build.
> > 
> > Cc: QI Fuli <qi.fuli@fujitsu.com>
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> > ---
> >  configure.ac                       |  9 ++++++++-
> >  daxctl/90-daxctl-device.rules      |  1 +
> >  daxctl/Makefile.am                 | 10 ++++++++++
> >  daxctl/daxdev-auto-reconfigure.sh  |  3 +++
> >  daxctl/daxdev-reconfigure@.service |  8 ++++++++
> >  ndctl.spec.in                      |  3 +++
> >  6 files changed, 33 insertions(+), 1 deletion(-)
> >  create mode 100644 daxctl/90-daxctl-device.rules
> >  create mode 100755 daxctl/daxdev-auto-reconfigure.sh
> >  create mode 100644 daxctl/daxdev-reconfigure@.service
> > 
> > diff --git a/configure.ac b/configure.ac
> > index 9e1c6db..df6ab10 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -160,7 +160,7 @@ AC_CHECK_FUNCS([ \
> > 
> >  AC_ARG_WITH([systemd],
> >         AS_HELP_STRING([--with-systemd],
> > -               [Enable systemd functionality (monitor). @<:@default=yes@:>@]),
> > +               [Enable systemd functionality. @<:@default=yes@:>@]),
> >         [], [with_systemd=yes])
> > 
> >  if test "x$with_systemd" = "xyes"; then
> > @@ -183,6 +183,13 @@ daxctl_modprobe_data=daxctl.conf
> >  AC_SUBST([daxctl_modprobe_datadir])
> >  AC_SUBST([daxctl_modprobe_data])
> > 
> > +AC_ARG_WITH(udevrulesdir,
> > +    [AS_HELP_STRING([--with-udevrulesdir=DIR], [udev rules.d directory])],
> > +    [UDEVRULESDIR="$withval"],
> > +    [UDEVRULESDIR='${prefix}/lib/udev/rules.d']
> > +)
> > +AC_SUBST(UDEVRULESDIR)
> > +
> >  AC_ARG_WITH([keyutils],
> >             AS_HELP_STRING([--with-keyutils],
> >                         [Enable keyutils functionality (security).  @<:@default=yes@:>@]), [], [with_keyutils=yes])
> > diff --git a/daxctl/90-daxctl-device.rules b/daxctl/90-daxctl-device.rules
> > new file mode 100644
> > index 0000000..ee0670f
> > --- /dev/null
> > +++ b/daxctl/90-daxctl-device.rules
> > @@ -0,0 +1 @@
> > +ACTION=="add", SUBSYSTEM=="dax", TAG+="systemd", ENV{SYSTEMD_WANTS}="daxdev-reconfigure@$env{DEVNAME}.service"
> > diff --git a/daxctl/Makefile.am b/daxctl/Makefile.am
> > index f30c485..d53bdcf 100644
> > --- a/daxctl/Makefile.am
> > +++ b/daxctl/Makefile.am
> > @@ -28,3 +28,13 @@ daxctl_LDADD =\
> >         $(UUID_LIBS) \
> >         $(KMOD_LIBS) \
> >         $(JSON_LIBS)
> > +
> > +bin_SCRIPTS = daxdev-auto-reconfigure.sh
> > +CLEANFILES = $(bin_SCRIPTS)
> > +
> > +udevrulesdir = $(UDEVRULESDIR)
> > +udevrules_DATA = 90-daxctl-device.rules
> > +
> > +if ENABLE_SYSTEMD_UNITS
> > +systemd_unit_DATA = daxdev-reconfigure@.service
> > +endif
> > diff --git a/daxctl/daxdev-auto-reconfigure.sh b/daxctl/daxdev-auto-reconfigure.sh
> > new file mode 100755
> > index 0000000..f6da43f
> > --- /dev/null
> > +++ b/daxctl/daxdev-auto-reconfigure.sh
> > @@ -0,0 +1,3 @@
> > +#!/bin/bash
> > +
> > +daxctl reconfigure-device --check-config "${1##*/}"
> > diff --git a/daxctl/daxdev-reconfigure@.service b/daxctl/daxdev-reconfigure@.service
> > new file mode 100644
> > index 0000000..451fef1
> > --- /dev/null
> > +++ b/daxctl/daxdev-reconfigure@.service
> > @@ -0,0 +1,8 @@
> > +[Unit]
> > +Description=Automatic daxctl device reconfiguration
> > +Documentation=man:daxctl-reconfigure-device(1)
> > +
> > +[Service]
> > +Type=forking
> > +GuessMainPID=false
> > +ExecStart=/bin/sh -c "exec daxdev-auto-reconfigure.sh %I"
> 
> Why is the daxdev-auto-reconfigure.sh indirection needed? Can this not be:
> 
> ExecStart=daxctl reconfigure-device -C %I
> 
> ...if the format of %l is the issue I think it would be good for
> reconfigure-device to be tolerant of this format.

Yeah it was the format of %I. I forget exactly what it was, I think it
contains maybe a full /dev/daxX.Y path? Since in the wrapper script I'm
clipping away everything upto the first '/'. Should we make
reconfigure-device understand /dev/daxX.Y?

Re: [ndctl PATCH 7/7] daxctl: add systemd service and udev rule for auto-onlining

[Dan Williams]

On Wed, Nov 17, 2021 at 3:29 PM Verma, Vishal L
<vishal.l.verma@intel.com> wrote:
>
> On Fri, 2021-09-17 at 11:10 -0700, Dan Williams wrote:
> > On Tue, Aug 31, 2021 at 2:05 AM Vishal Verma <vishal.l.verma@intel.com> wrote:
> > >
> > > Install a helper script that calls daxctl-reconfigure-device with the
> > > new 'check-config' option for a given device. This is meant to be called
> > > via a systemd service.
> > >
> > > Install a systemd service that calls the above wrapper script with a
> > > daxctl device passed in to it via the environment.
> > >
> > > Install a udev rule that is triggered for every daxctl device, and
> > > triggers the above oneshot systemd service.
> > >
> > > Together, these three things work such that upon boot, whenever a daxctl
> > > device is found, udev triggers a device-specific systemd service called,
> > > for example:
> > >
> > >   daxdev-reconfigure@-dev-dax0.0.service
> >
> > I'm thinking the service would be called daxdev-add, because it is
> > servicing KOBJ_ADD events, or is the convention to name the service
> > after what it does vs what it services?
> >
> > Also, I'm curious why would "dax0.0" be in the service name, shouldn't
> > this be scanning all dax devices and internally matching based on the
> > config file?
> >
> > >
> > > This initiates a daxctl-reconfigure-device with a config lookup for the
> > > 'dax0.0' device. If the config has an '[auto-online <unique_id>]'
> > > section, it uses the information in that to set the operating mode of
> > > the device.
> > >
> > > If any device is in an unexpected status, 'journalctl' can be used to
> > > view the reconfiguration log for that device, for example:
> > >
> > >   journalctl --unit daxdev-reconfigure@-dev-dax0.0.service
> >
> > There will be a log per-device, or only if there is a service per
> > device? My assumption was that this service is firing off for all
> > devices so you would need to filter the log by the device-name if you
> > know it... or maybe I'm misunderstanding how this udev service works.
> >
> > >
> > > Update the RPM spec file to include the newly added files to the RPM
> > > build.
> > >
> > > Cc: QI Fuli <qi.fuli@fujitsu.com>
> > > Cc: Dan Williams <dan.j.williams@intel.com>
> > > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> > > ---
> > >  configure.ac                       |  9 ++++++++-
> > >  daxctl/90-daxctl-device.rules      |  1 +
> > >  daxctl/Makefile.am                 | 10 ++++++++++
> > >  daxctl/daxdev-auto-reconfigure.sh  |  3 +++
> > >  daxctl/daxdev-reconfigure@.service |  8 ++++++++
> > >  ndctl.spec.in                      |  3 +++
> > >  6 files changed, 33 insertions(+), 1 deletion(-)
> > >  create mode 100644 daxctl/90-daxctl-device.rules
> > >  create mode 100755 daxctl/daxdev-auto-reconfigure.sh
> > >  create mode 100644 daxctl/daxdev-reconfigure@.service
> > >
> > > diff --git a/configure.ac b/configure.ac
> > > index 9e1c6db..df6ab10 100644
> > > --- a/configure.ac
> > > +++ b/configure.ac
> > > @@ -160,7 +160,7 @@ AC_CHECK_FUNCS([ \
> > >
> > >  AC_ARG_WITH([systemd],
> > >         AS_HELP_STRING([--with-systemd],
> > > -               [Enable systemd functionality (monitor). @<:@default=yes@:>@]),
> > > +               [Enable systemd functionality. @<:@default=yes@:>@]),
> > >         [], [with_systemd=yes])
> > >
> > >  if test "x$with_systemd" = "xyes"; then
> > > @@ -183,6 +183,13 @@ daxctl_modprobe_data=daxctl.conf
> > >  AC_SUBST([daxctl_modprobe_datadir])
> > >  AC_SUBST([daxctl_modprobe_data])
> > >
> > > +AC_ARG_WITH(udevrulesdir,
> > > +    [AS_HELP_STRING([--with-udevrulesdir=DIR], [udev rules.d directory])],
> > > +    [UDEVRULESDIR="$withval"],
> > > +    [UDEVRULESDIR='${prefix}/lib/udev/rules.d']
> > > +)
> > > +AC_SUBST(UDEVRULESDIR)
> > > +
> > >  AC_ARG_WITH([keyutils],
> > >             AS_HELP_STRING([--with-keyutils],
> > >                         [Enable keyutils functionality (security).  @<:@default=yes@:>@]), [], [with_keyutils=yes])
> > > diff --git a/daxctl/90-daxctl-device.rules b/daxctl/90-daxctl-device.rules
> > > new file mode 100644
> > > index 0000000..ee0670f
> > > --- /dev/null
> > > +++ b/daxctl/90-daxctl-device.rules
> > > @@ -0,0 +1 @@
> > > +ACTION=="add", SUBSYSTEM=="dax", TAG+="systemd", ENV{SYSTEMD_WANTS}="daxdev-reconfigure@$env{DEVNAME}.service"
> > > diff --git a/daxctl/Makefile.am b/daxctl/Makefile.am
> > > index f30c485..d53bdcf 100644
> > > --- a/daxctl/Makefile.am
> > > +++ b/daxctl/Makefile.am
> > > @@ -28,3 +28,13 @@ daxctl_LDADD =\
> > >         $(UUID_LIBS) \
> > >         $(KMOD_LIBS) \
> > >         $(JSON_LIBS)
> > > +
> > > +bin_SCRIPTS = daxdev-auto-reconfigure.sh
> > > +CLEANFILES = $(bin_SCRIPTS)
> > > +
> > > +udevrulesdir = $(UDEVRULESDIR)
> > > +udevrules_DATA = 90-daxctl-device.rules
> > > +
> > > +if ENABLE_SYSTEMD_UNITS
> > > +systemd_unit_DATA = daxdev-reconfigure@.service
> > > +endif
> > > diff --git a/daxctl/daxdev-auto-reconfigure.sh b/daxctl/daxdev-auto-reconfigure.sh
> > > new file mode 100755
> > > index 0000000..f6da43f
> > > --- /dev/null
> > > +++ b/daxctl/daxdev-auto-reconfigure.sh
> > > @@ -0,0 +1,3 @@
> > > +#!/bin/bash
> > > +
> > > +daxctl reconfigure-device --check-config "${1##*/}"
> > > diff --git a/daxctl/daxdev-reconfigure@.service b/daxctl/daxdev-reconfigure@.service
> > > new file mode 100644
> > > index 0000000..451fef1
> > > --- /dev/null
> > > +++ b/daxctl/daxdev-reconfigure@.service
> > > @@ -0,0 +1,8 @@
> > > +[Unit]
> > > +Description=Automatic daxctl device reconfiguration
> > > +Documentation=man:daxctl-reconfigure-device(1)
> > > +
> > > +[Service]
> > > +Type=forking
> > > +GuessMainPID=false
> > > +ExecStart=/bin/sh -c "exec daxdev-auto-reconfigure.sh %I"
> >
> > Why is the daxdev-auto-reconfigure.sh indirection needed? Can this not be:
> >
> > ExecStart=daxctl reconfigure-device -C %I
> >
> > ...if the format of %l is the issue I think it would be good for
> > reconfigure-device to be tolerant of this format.
>
> Yeah it was the format of %I. I forget exactly what it was, I think it
> contains maybe a full /dev/daxX.Y path? Since in the wrapper script I'm
> clipping away everything upto the first '/'. Should we make
> reconfigure-device understand /dev/daxX.Y?

Yeah, I think it follows the principle of least surprise if:

daxctl reconfigure-device /dev/daxX.Y

...and:

daxctl reconfigure-device daxX.Y

...behave the same.

Re: [ndctl PATCH 6/7] daxctl/device.c: add an option for getting params from a config file

[Verma, Vishal L]

On Thu, 2021-09-16 at 18:59 -0700, Dan Williams wrote:
> On Tue, Aug 31, 2021 at 2:05 AM Vishal Verma <vishal.l.verma@intel.com> wrote:
> > 
> > Add a new option to daxctl-reconfigure-device that allows it to
> > comprehend the new global config system in ndctl/daxctl. With this, the
> > reconfigure-device command can query the config to match a specific
> > device UUID, and operate using the parameters supplied in that INI
> > section.
> > 
> > This is in preparation to make daxctl device reconfiguration (usually
> > as system-ram) policy based, so that reconfiguration can happen
> > automatically on boot.
> > 
> > Cc: QI Fuli <qi.fuli@fujitsu.com>
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> > ---
> >  daxctl/daxctl.c    |   1 +
> >  daxctl/device.c    | 141 ++++++++++++++++++++++++++++++++++++++++++++-
> >  daxctl/Makefile.am |   1 +
> >  3 files changed, 142 insertions(+), 1 deletion(-)
> > 
> > diff --git a/daxctl/daxctl.c b/daxctl/daxctl.c
> > index 928814c..dc7ac5f 100644
> > --- a/daxctl/daxctl.c
> > +++ b/daxctl/daxctl.c
> > @@ -95,6 +95,7 @@ int main(int argc, const char **argv)
> >         rc = daxctl_new(&ctx);
> >         if (rc)
> >                 goto out;
> > +       daxctl_set_configs(&ctx, DAXCTL_CONF_DIR);
> 
> Now that I see how this is used, I think this wants to be called:
> 
> daxctl_ctx_add_configs()
> 
> ...because the operation is populating the context with all the
> configuration fragments found in the given directory.
> 
> I would also have an option to specify an override for the config
> directory, maybe that could be an environment variable?

Hm, the concern with _add_configs is that is sounds like we're
additively adding multiple directories containing config files, which
isn't true - this (currently at least) supports only one directory at a
time - hence 'set'.

Maybe daxctl_set_configs_dir() is more appropriate?

> 
> >         main_handle_internal_command(argc, argv, ctx, commands,
> >                         ARRAY_SIZE(commands), PROG_DAXCTL);
> >         daxctl_unref(ctx);
> > diff --git a/daxctl/device.c b/daxctl/device.c
> > index a427b7d..99a4548 100644
> > --- a/daxctl/device.c
> > +++ b/daxctl/device.c
> > @@ -14,8 +14,10 @@
> >  #include <util/filter.h>
> >  #include <json-c/json.h>
> >  #include <json-c/json_util.h>
> > +#include <ndctl/libndctl.h>
> >  #include <daxctl/libdaxctl.h>
> >  #include <util/parse-options.h>
> > +#include <util/parse-configs.h>
> >  #include <ccan/array_size/array_size.h>
> > 
> >  static struct {
> > @@ -25,6 +27,7 @@ static struct {
> >         const char *size;
> >         const char *align;
> >         const char *input;
> > +       bool check_config;
> >         bool no_online;
> >         bool no_movable;
> >         bool force;
> > @@ -75,7 +78,9 @@ OPT_STRING('m', "mode", &param.mode, "mode", "mode to switch the device to"), \
> >  OPT_BOOLEAN('N', "no-online", &param.no_online, \
> >         "don't auto-online memory sections"), \
> >  OPT_BOOLEAN('f', "force", &param.force, \
> > -               "attempt to offline memory sections before reconfiguration")
> > +               "attempt to offline memory sections before reconfiguration"), \
> > +OPT_BOOLEAN('C', "check-config", &param.check_config, \
> > +               "use config files to determine parameters for the operation")
> > 
> >  #define CREATE_OPTIONS() \
> >  OPT_STRING('s', "size", &param.size, "size", "size to switch the device to"), \
> > @@ -218,6 +223,130 @@ err:
> >         return rc;
> >  }
> > 
> > +static int conf_string_to_bool(const char *str)
> > +{
> > +       if (!str)
> > +               return INT_MAX;
> > +       if (strncmp(str, "t", 1) == 0 ||
> > +                       strncmp(str, "T", 1) == 0 ||
> > +                       strncmp(str, "y", 1) == 0 ||
> > +                       strncmp(str, "Y", 1) == 0 ||
> > +                       strncmp(str, "1", 1) == 0)
> > +               return true;
> > +       if (strncmp(str, "f", 1) == 0 ||
> > +                       strncmp(str, "F", 1) == 0 ||
> > +                       strncmp(str, "n", 1) == 0 ||
> > +                       strncmp(str, "N", 1) == 0 ||
> > +                       strncmp(str, "0", 1) == 0)
> > +               return false;
> 
> Doesn't iniparser_getboolean() already do this conversion for you?

It does.. but the API provided by parse-configs.h, specifically the
CONF_SEARCH macro only deals in strings. I would probably need to add a
CONF_SEARCH_BOOL to return booleans, that then internally uses
iniparser_getboolean.

> 
> > +       return INT_MAX;
> > +}
> > +
> > +#define conf_assign_inverted_bool(p, conf_var) \
> > +do { \
> > +       if (conf_string_to_bool(conf_var) != INT_MAX) \
> > +               param.p = !conf_string_to_bool(conf_var); \
> > +} while(0)
> > +
> > +static int parse_config_reconfig_set_params(struct daxctl_ctx *ctx, const char *device,
> > +                                           const char *uuid)
> > +{
> > +       const char *conf_online = NULL, *conf_movable = NULL;
> > +       const struct config configs[] = {
> > +               CONF_SEARCH("auto-online", "uuid", uuid, "mode", &param.mode, NULL),
> > +               CONF_SEARCH("auto-online", "uuid", uuid, "online", &conf_online, NULL),
> > +               CONF_SEARCH("auto-online", "uuid", uuid, "movable", &conf_movable, NULL),
> > +               CONF_END(),
> > +       };
> > +       const char *prefix = "./";
> > +       int rc;
> > +
> > +       rc = parse_configs_prefix(daxctl_get_configs(ctx), prefix, configs);
> > +       if (rc < 0)
> > +               return rc;
> > +
> > +       conf_assign_inverted_bool(no_online, conf_online);
> > +       conf_assign_inverted_bool(no_movable, conf_movable);
> > +
> > +       return 0;
> > +}
> > +
> > +static bool daxctl_ndns_has_device(struct ndctl_namespace *ndns,
> > +                                   const char *device)
> > +{
> > +       struct daxctl_region *dax_region;
> > +       struct ndctl_dax *dax;
> > +
> > +       dax = ndctl_namespace_get_dax(ndns);
> > +       if (!dax)
> > +               return false;
> > +
> > +       dax_region = ndctl_dax_get_daxctl_region(dax);
> > +       if (dax_region) {
> > +               struct daxctl_dev *dev;
> > +
> > +               dev = daxctl_dev_get_first(dax_region);
> > +               if (dev) {
> > +                       if (strcmp(daxctl_dev_get_devname(dev), device) == 0)
> > +                               return true;
> > +               }
> > +       }
> > +       return false;
> > +}
> > +
> > +static int parse_config_reconfig(struct daxctl_ctx *ctx, const char *device)
> > +{
> > +       struct ndctl_namespace *ndns;
> > +       struct ndctl_ctx *ndctl_ctx;
> > +       struct ndctl_region *region;
> > +       struct ndctl_bus *bus;
> > +       struct ndctl_dax *dax;
> > +       int rc, found = 0;
> > +       char uuid_buf[40];
> > +       uuid_t uuid;
> > +
> > +       if (strcmp(device, "all") == 0)
> > +               return 0;
> > +
> > +       rc = ndctl_new(&ndctl_ctx);
> > +       if (rc < 0)
> > +               return rc;
> > +
> > +        ndctl_bus_foreach(ndctl_ctx, bus) {
> > +               ndctl_region_foreach(bus, region) {
> > +                       ndctl_namespace_foreach(region, ndns) {
> > +                               if (daxctl_ndns_has_device(ndns, device)) {
> > +                                       dax = ndctl_namespace_get_dax(ndns);
> > +                                       if (!dax)
> > +                                               continue;
> > +                                       ndctl_dax_get_uuid(dax, uuid);
> > +                                       found = 1;
> > +                               }
> > +                       }
> > +               }
> > +       }
> > +
> > +       if (!found) {
> > +               fprintf(stderr, "no UUID match for %s found in config files\n",
> > +                       device);
> > +               return 0;
> > +       }
> > +
> > +       uuid_unparse(uuid, uuid_buf);
> > +       return parse_config_reconfig_set_params(ctx, device, uuid_buf);
> > +}
> > +
> > +static int parse_device_config(struct daxctl_ctx *ctx, const char *device,
> > +                              enum device_action action)
> > +{
> > +       switch (action) {
> > +       case ACTION_RECONFIG:
> > +               return parse_config_reconfig(ctx, device);
> > +       default:
> > +               return 0;
> > +       }
> > +}
> > +
> >  static const char *parse_device_options(int argc, const char **argv,
> >                 enum device_action action, const struct option *options,
> >                 const char *usage, struct daxctl_ctx *ctx)
> > @@ -279,6 +408,16 @@ static const char *parse_device_options(int argc, const char **argv,
> >         if (param.human)
> >                 flags |= UTIL_JSON_HUMAN;
> > 
> > +       /* Scan config file(s) for options. This sets param.foo accordingly */
> > +       if (param.check_config) {
> > +               rc = parse_device_config(ctx, argv[0], action);
> 
> What happens if someone does:
> 
> daxctl reconfigure-device -C --no-online /dev/dax0.0
> 
> ...and it matches an entry in the configuration file that has
> "system-ram.online = true".
> 
> My expectation is that precedence ordering is:
> 
> built-in defaults
> configuration file settings
> command line options
> 
> Where later entries in that list override settings from the previous
> entry. That said, I don't see an easy way to achieve this with parse
> options, so might need to fail if anything but -C is specified as an
> option until we can fix that conflict.

Yeah I struggled with this too :)  The precedence ordering makes sense,
and I wanted to implement that as well, but it wasn't wasily possible
with the param stuffing without a larger rework. Bailing on a mismatch
makes sense though, I'll add that.

> 
> > +               if (rc) {
> > +                       fprintf(stderr, "error parsing config file: %s\n",
> > +                               strerror(-rc));
> > +                       return NULL;
> > +               }
> > +       }
> > +
> >         /* Handle action-specific options */
> >         switch (action) {
> >         case ACTION_RECONFIG:
> > diff --git a/daxctl/Makefile.am b/daxctl/Makefile.am
> > index a9845a0..f30c485 100644
> > --- a/daxctl/Makefile.am
> > +++ b/daxctl/Makefile.am
> > @@ -23,6 +23,7 @@ daxctl_SOURCES =\
> > 
> >  daxctl_LDADD =\
> >         lib/libdaxctl.la \
> > +       ../ndctl/lib/libndctl.la \
> >         ../libutil.a \
> >         $(UUID_LIBS) \
> >         $(KMOD_LIBS) \
> > --
> > 2.31.1
> > 

Re: [ndctl PATCH 7/7] daxctl: add systemd service and udev rule for auto-onlining

[Verma, Vishal L]

On Fri, 2021-09-17 at 11:10 -0700, Dan Williams wrote:
> On Tue, Aug 31, 2021 at 2:05 AM Vishal Verma <vishal.l.verma@intel.com> wrote:
> > 
> > Install a helper script that calls daxctl-reconfigure-device with the
> > new 'check-config' option for a given device. This is meant to be called
> > via a systemd service.
> > 
> > Install a systemd service that calls the above wrapper script with a
> > daxctl device passed in to it via the environment.
> > 
> > Install a udev rule that is triggered for every daxctl device, and
> > triggers the above oneshot systemd service.
> > 
> > Together, these three things work such that upon boot, whenever a daxctl
> > device is found, udev triggers a device-specific systemd service called,
> > for example:
> > 
> >   daxdev-reconfigure@-dev-dax0.0.service
> 
> I'm thinking the service would be called daxdev-add, because it is
> servicing KOBJ_ADD events, or is the convention to name the service
> after what it does vs what it services?

I don't know of a convention - but 'what it does' seemed more natural
for a service than 'when it's called'. It also correlates better with
usual system service names (i.e. they are named after what they do).

> 
> Also, I'm curious why would "dax0.0" be in the service name, shouldn't
> this be scanning all dax devices and internally matching based on the
> config file?

Systemd black magic? the dax0.0 doesn't come from anything I configure
- that's just how systemd's 'instantiated services' work. Each newly
added device gets it's own service tied to a unique identifier for the
device. For these, it happens to be /dev/dax0.0, which gets escaped to
-dev-dax0.0.

More reading here:
http://0pointer.de/blog/projects/instances.html

> 
> > 
> > This initiates a daxctl-reconfigure-device with a config lookup for the
> > 'dax0.0' device. If the config has an '[auto-online <unique_id>]'
> > section, it uses the information in that to set the operating mode of
> > the device.
> > 
> > If any device is in an unexpected status, 'journalctl' can be used to
> > view the reconfiguration log for that device, for example:
> > 
> >   journalctl --unit daxdev-reconfigure@-dev-dax0.0.service
> 
> There will be a log per-device, or only if there is a service per
> device? My assumption was that this service is firing off for all
> devices so you would need to filter the log by the device-name if you
> know it... or maybe I'm misunderstanding how this udev service works.

There will be both a log and a service per device - the unit file we
supply/install is essentially a template for these instantiated
services, but the actual service kicked off is <foo>@<device-
id>.service

> 
> > 
> > Update the RPM spec file to include the newly added files to the RPM
> > build.
> > 
> > Cc: QI Fuli <qi.fuli@fujitsu.com>
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> > ---
> >  configure.ac                       |  9 ++++++++-
> >  daxctl/90-daxctl-device.rules      |  1 +
> >  daxctl/Makefile.am                 | 10 ++++++++++
> >  daxctl/daxdev-auto-reconfigure.sh  |  3 +++
> >  daxctl/daxdev-reconfigure@.service |  8 ++++++++
> >  ndctl.spec.in                      |  3 +++
> >  6 files changed, 33 insertions(+), 1 deletion(-)
> >  create mode 100644 daxctl/90-daxctl-device.rules
> >  create mode 100755 daxctl/daxdev-auto-reconfigure.sh
> >  create mode 100644 daxctl/daxdev-reconfigure@.service
> > 
> > diff --git a/configure.ac b/configure.ac
> > index 9e1c6db..df6ab10 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -160,7 +160,7 @@ AC_CHECK_FUNCS([ \
> > 
> >  AC_ARG_WITH([systemd],
> >         AS_HELP_STRING([--with-systemd],
> > -               [Enable systemd functionality (monitor). @<:@default=yes@:>@]),
> > +               [Enable systemd functionality. @<:@default=yes@:>@]),
> >         [], [with_systemd=yes])
> > 
> >  if test "x$with_systemd" = "xyes"; then
> > @@ -183,6 +183,13 @@ daxctl_modprobe_data=daxctl.conf
> >  AC_SUBST([daxctl_modprobe_datadir])
> >  AC_SUBST([daxctl_modprobe_data])
> > 
> > +AC_ARG_WITH(udevrulesdir,
> > +    [AS_HELP_STRING([--with-udevrulesdir=DIR], [udev rules.d directory])],
> > +    [UDEVRULESDIR="$withval"],
> > +    [UDEVRULESDIR='${prefix}/lib/udev/rules.d']
> > +)
> > +AC_SUBST(UDEVRULESDIR)
> > +
> >  AC_ARG_WITH([keyutils],
> >             AS_HELP_STRING([--with-keyutils],
> >                         [Enable keyutils functionality (security).  @<:@default=yes@:>@]), [], [with_keyutils=yes])
> > diff --git a/daxctl/90-daxctl-device.rules b/daxctl/90-daxctl-device.rules
> > new file mode 100644
> > index 0000000..ee0670f
> > --- /dev/null
> > +++ b/daxctl/90-daxctl-device.rules
> > @@ -0,0 +1 @@
> > +ACTION=="add", SUBSYSTEM=="dax", TAG+="systemd", ENV{SYSTEMD_WANTS}="daxdev-reconfigure@$env{DEVNAME}.service"
> > diff --git a/daxctl/Makefile.am b/daxctl/Makefile.am
> > index f30c485..d53bdcf 100644
> > --- a/daxctl/Makefile.am
> > +++ b/daxctl/Makefile.am
> > @@ -28,3 +28,13 @@ daxctl_LDADD =\
> >         $(UUID_LIBS) \
> >         $(KMOD_LIBS) \
> >         $(JSON_LIBS)
> > +
> > +bin_SCRIPTS = daxdev-auto-reconfigure.sh
> > +CLEANFILES = $(bin_SCRIPTS)
> > +
> > +udevrulesdir = $(UDEVRULESDIR)
> > +udevrules_DATA = 90-daxctl-device.rules
> > +
> > +if ENABLE_SYSTEMD_UNITS
> > +systemd_unit_DATA = daxdev-reconfigure@.service
> > +endif
> > diff --git a/daxctl/daxdev-auto-reconfigure.sh b/daxctl/daxdev-auto-reconfigure.sh
> > new file mode 100755
> > index 0000000..f6da43f
> > --- /dev/null
> > +++ b/daxctl/daxdev-auto-reconfigure.sh
> > @@ -0,0 +1,3 @@
> > +#!/bin/bash
> > +
> > +daxctl reconfigure-device --check-config "${1##*/}"
> > diff --git a/daxctl/daxdev-reconfigure@.service b/daxctl/daxdev-reconfigure@.service
> > new file mode 100644
> > index 0000000..451fef1
> > --- /dev/null
> > +++ b/daxctl/daxdev-reconfigure@.service
> > @@ -0,0 +1,8 @@
> > +[Unit]
> > +Description=Automatic daxctl device reconfiguration
> > +Documentation=man:daxctl-reconfigure-device(1)
> > +
> > +[Service]
> > +Type=forking
> > +GuessMainPID=false
> > +ExecStart=/bin/sh -c "exec daxdev-auto-reconfigure.sh %I"
> 
> Why is the daxdev-auto-reconfigure.sh indirection needed? Can this not be:
> 
> ExecStart=daxctl reconfigure-device -C %I
> 
> ...if the format of %l is the issue I think it would be good for
> reconfigure-device to be tolerant of this format.

Re: [ndctl PATCH 7/7] daxctl: add systemd service and udev rule for auto-onlining

[Dan Williams]

On Wed, Nov 17, 2021 at 6:40 PM Verma, Vishal L
<vishal.l.verma@intel.com> wrote:
>
> On Fri, 2021-09-17 at 11:10 -0700, Dan Williams wrote:
> > On Tue, Aug 31, 2021 at 2:05 AM Vishal Verma <vishal.l.verma@intel.com> wrote:
> > >
> > > Install a helper script that calls daxctl-reconfigure-device with the
> > > new 'check-config' option for a given device. This is meant to be called
> > > via a systemd service.
> > >
> > > Install a systemd service that calls the above wrapper script with a
> > > daxctl device passed in to it via the environment.
> > >
> > > Install a udev rule that is triggered for every daxctl device, and
> > > triggers the above oneshot systemd service.
> > >
> > > Together, these three things work such that upon boot, whenever a daxctl
> > > device is found, udev triggers a device-specific systemd service called,
> > > for example:
> > >
> > >   daxdev-reconfigure@-dev-dax0.0.service
> >
> > I'm thinking the service would be called daxdev-add, because it is
> > servicing KOBJ_ADD events, or is the convention to name the service
> > after what it does vs what it services?
>
> I don't know of a convention - but 'what it does' seemed more natural
> for a service than 'when it's called'. It also correlates better with
> usual system service names (i.e. they are named after what they do).
>
> >
> > Also, I'm curious why would "dax0.0" be in the service name, shouldn't
> > this be scanning all dax devices and internally matching based on the
> > config file?
>
> Systemd black magic? the dax0.0 doesn't come from anything I configure
> - that's just how systemd's 'instantiated services' work. Each newly
> added device gets it's own service tied to a unique identifier for the
> device. For these, it happens to be /dev/dax0.0, which gets escaped to
> -dev-dax0.0.
>
> More reading here:
> http://0pointer.de/blog/projects/instances.html
>
> >
> > >
> > > This initiates a daxctl-reconfigure-device with a config lookup for the
> > > 'dax0.0' device. If the config has an '[auto-online <unique_id>]'
> > > section, it uses the information in that to set the operating mode of
> > > the device.
> > >
> > > If any device is in an unexpected status, 'journalctl' can be used to
> > > view the reconfiguration log for that device, for example:
> > >
> > >   journalctl --unit daxdev-reconfigure@-dev-dax0.0.service
> >
> > There will be a log per-device, or only if there is a service per
> > device? My assumption was that this service is firing off for all
> > devices so you would need to filter the log by the device-name if you
> > know it... or maybe I'm misunderstanding how this udev service works.
>
> There will be both a log and a service per device - the unit file we
> supply/install is essentially a template for these instantiated
> services, but the actual service kicked off is <foo>@<device-
> id>.service

Ah, ok, that makes sense.

Re: [ndctl PATCH 0/7] Policy based reconfiguration for daxctl

[Verma, Vishal L]

On Thu, 2021-09-16 at 15:12 -0700, Dan Williams wrote:
> On Tue, Aug 31, 2021 at 2:05 AM Vishal Verma <vishal.l.verma@intel.com> wrote:
> > 
> > These patches add policy (config file) support to daxctl. The
> > introductory user is daxctl-reconfigure-device. Sysadmins may wish to
> > use daxctl devices as system-ram, but it may be cumbersome to automate
> > the reconfiguration step for every device upon boot.
> > 
> > Introduce a new option for daxctl-reconfigure-device, --check-config.
> > This is at the heart of policy based reconfiguration, as it allows
> > daxctl to look up reconfiguration parameters for a given device from the
> > config system instead of the command line.
> > 
> > Some systemd and udev glue then automates this for every new dax device
> > that shows up, providing a way for the administrator to simply list all
> > the 'system-ram' UUIDs in a config file, and not have to worry about
> > anything else.
> > 
> > An example config file can be:
> > 
> >   # cat /etc/ndctl/daxctl.conf
> 
> Take these comments as provisional until I read through the rest, but
> this is just a reaction to the proposed ini format.

I somehow missed this email originally, and just saw it on lore..

> > 
> >   [auto-online unique_identifier_foo]
> 
> I am thinking this section name should be "reconfigure-device
> unique_identifier_foo" if only because resize might also be something
> someone wants to do, and if other commands get config automation it
> makes it clearer which config snippets apply to which command.

Yep that makes sense - I'll change this.
> 
> >   uuid = 48d8e42c-a2f0-4312-9e70-a837faafe862
> 
> I think this should be called:
> 
> "nvdimm.uuid"
> 
> ...or something like that to make it clear this depends on dax devices
> emitted by libnvdimm, and not those that come from "soft-reserved"
> memory. It also helps distinguish if we ever get UUIDs in the HMAT
> which is something I have been meaning to propose.

Yep makes sense, will change.

> >   mode = system-ram
> 
> I can see this being "mode = devdax" if feature was being used to
> change size or alignment.

Agreed, but that should 'just work' right - especially once we rename
the section name from auto-online to reconfigure-device.

> 
> >   online = true
> >   movable = false
> 
> I wonder if these keys should be prefixed by the mode name:
> 
> system-ram.online = true
> system-ram.movable = false

Hm, maybe, but since the config options feed directly into the commands
params, I figured we can let the command's option parsing throw any
errors for incompatible options. My hope was for config identifiers to
- as far as possible - exactly match command-line options. If we do
make changes like this, I feel for every command that supports config,
the man page is asking for a dedicated config section documenting every
config option it supports. Maybe this is a good idea regardless :)

> 
> ...so it's a bit more self documenting about which parameters are
> sub-options, and delineates them from generic options like size.
> 
> > Any file under '/etc/ndctl/' can be used - all files with a '.conf' suffix
> > will be considered when looking for matches.
> 
> any concern about name collisions between ndctl, daxctl, and cxl-cli
> section names?

Yep good point, changed this to be in /etc/daxctl/, and cxl-cli can get
its own directory too when the time comes.