From: "Verma, Vishal L" <vishal.l.verma@intel.com>
To: "Williams, Dan J" <dan.j.williams@intel.com>
Cc: "nvdimm@lists.linux.dev" <nvdimm@lists.linux.dev>,
"qi.fuli@fujitsu.com" <qi.fuli@fujitsu.com>,
"Hu, Fenghua" <fenghua.hu@intel.com>,
"qi.fuli@jp.fujitsu.com" <qi.fuli@jp.fujitsu.com>
Subject: Re: [ndctl PATCH 6/7] daxctl/device.c: add an option for getting params from a config file
Date: Wed, 17 Nov 2021 23:45:51 +0000 [thread overview]
Message-ID: <24cc446d9c93ed7de7eabbb038ca8b29cd6cde37.camel@intel.com> (raw)
In-Reply-To: <CAPcyv4iR3CSPBQrcU6JLLqKRaAChcfd=REQR+1NEthNZsDmTBQ@mail.gmail.com>
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", ¶m.mode, "mode", "mode to switch the device to"), \
> > OPT_BOOLEAN('N', "no-online", ¶m.no_online, \
> > "don't auto-online memory sections"), \
> > OPT_BOOLEAN('f', "force", ¶m.force, \
> > - "attempt to offline memory sections before reconfiguration")
> > + "attempt to offline memory sections before reconfiguration"), \
> > +OPT_BOOLEAN('C', "check-config", ¶m.check_config, \
> > + "use config files to determine parameters for the operation")
> >
> > #define CREATE_OPTIONS() \
> > OPT_STRING('s', "size", ¶m.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", ¶m.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
> >
next prev parent reply other threads:[~2021-11-17 23:46 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-31 9:04 [ndctl PATCH 0/7] Policy based reconfiguration for daxctl Vishal Verma
2021-08-31 9:04 ` [ndctl PATCH 1/7] ndctl: Update ndctl.spec.in for 'ndctl.conf' Vishal Verma
2021-09-02 12:15 ` qi.fuli
2021-08-31 9:04 ` [ndctl PATCH 2/7] daxctl: Documentation updates for persistent reconfiguration Vishal Verma
2021-09-16 22:47 ` Dan Williams
2021-11-17 23:02 ` Verma, Vishal L
2021-08-31 9:04 ` [ndctl PATCH 3/7] util/parse-config: refactor filter_conf_files into util/ Vishal Verma
2021-09-02 12:17 ` qi.fuli
2021-09-16 22:54 ` Dan Williams
2021-08-31 9:04 ` [ndctl PATCH 4/7] daxctl: add basic config parsing support Vishal Verma
2021-09-02 12:19 ` qi.fuli
2021-09-16 22:58 ` Dan Williams
2021-11-17 23:17 ` Verma, Vishal L
2021-08-31 9:04 ` [ndctl PATCH 5/7] util/parse-configs: add a key/value search helper Vishal Verma
2021-09-02 13:12 ` qi.fuli
2021-09-16 23:54 ` Dan Williams
2021-11-17 23:21 ` Verma, Vishal L
2021-08-31 9:04 ` [ndctl PATCH 6/7] daxctl/device.c: add an option for getting params from a config file Vishal Verma
2021-09-17 1:59 ` Dan Williams
2021-11-17 23:45 ` Verma, Vishal L [this message]
2021-08-31 9:04 ` [ndctl PATCH 7/7] daxctl: add systemd service and udev rule for auto-onlining Vishal Verma
2021-09-03 0:56 ` qi.fuli
2021-09-17 18:10 ` Dan Williams
2021-11-17 23:29 ` Verma, Vishal L
2021-11-17 23:43 ` Dan Williams
2021-11-18 2:40 ` Verma, Vishal L
2021-11-18 3:40 ` Dan Williams
2021-09-16 22:12 ` [ndctl PATCH 0/7] Policy based reconfiguration for daxctl Dan Williams
2021-11-19 20:57 ` Verma, Vishal L
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=24cc446d9c93ed7de7eabbb038ca8b29cd6cde37.camel@intel.com \
--to=vishal.l.verma@intel.com \
--cc=dan.j.williams@intel.com \
--cc=fenghua.hu@intel.com \
--cc=nvdimm@lists.linux.dev \
--cc=qi.fuli@fujitsu.com \
--cc=qi.fuli@jp.fujitsu.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).