On Tue, 2021-12-07 at 14:51 -0800, Dan Williams wrote:
> On Mon, Dec 6, 2021 at 2:28 PM Vishal Verma <[email protected]> wrote:
> >
> > From: QI Fuli <[email protected]>
> >
> > Add ndctl_configs to ndctl_ctx for supporting ndctl global configuration
> > files. All files with .conf suffix under {sysconfdir}/ndctl can be
>
> I expect this should be named a ".d" directory to match expectations
> of when a directory supports multiple config-file snippets. Either
> "{sysconfdir}/ndctl/ndctl.d", "{sysconfdir}/ndctl/conf.d",
> ""{sysconfdir}/ndctl.d" would be ok with me. "{sysconfdir}/ndctl" is
> still needed for the security keys.
".d" sounds good - I prefer "{sysconfdir}/ndctl.d" or even something
like "{sysconfdir}/ndctl.conf.d" to make it explicit that this is for
config files.
>
> > regarded as global configuration files that all ndctl commands can refer
> > to. Add ndctl_set_configs() public function for setting ndctl default
> > configuration files' path. Add ndctl_get_configs() public function for
> > getting ndctl configuration files' path form ndctl_ctx.
>
> s/form/from/
>
> Lets those ndctl_{get,set}_config_path().
>
>
> >
> > Link: https://lore.kernel.org/r/[email protected]
> > Signed-off-by: QI Fuli <[email protected]>
> > Signed-off-by: Vishal Verma <[email protected]>
> > ---
> > configure.ac | 4 ++--
> > ndctl/lib/private.h | 1 +
> > ndctl/lib/libndctl.c | 54 ++++++++++++++++++++++++++++++++++++++++++
> > ndctl/libndctl.h | 2 ++
> > ndctl/ndctl.c | 1 +
> > ndctl/Makefile.am | 5 ++--
> > ndctl/lib/Makefile.am | 4 ++++
> > ndctl/lib/libndctl.sym | 2 ++
> > 8 files changed, 69 insertions(+), 4 deletions(-)
> >
> > diff --git a/configure.ac b/configure.ac
> > index dc39dbe..42a66e1 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -171,9 +171,9 @@ fi
> > AC_SUBST([systemd_unitdir])
> > AM_CONDITIONAL([ENABLE_SYSTEMD_UNITS], [test "x$with_systemd" = "xyes"])
> >
> > -ndctl_monitorconfdir=${sysconfdir}/ndctl
> > +ndctl_confdir=${sysconfdir}/ndctl
> > ndctl_monitorconf=monitor.conf
> > -AC_SUBST([ndctl_monitorconfdir])
> > +AC_SUBST([ndctl_confdir])
> > AC_SUBST([ndctl_monitorconf])
> >
> > daxctl_modprobe_datadir=${datadir}/daxctl
> > diff --git a/ndctl/lib/private.h b/ndctl/lib/private.h
> > index 8f4510e..f4ca71f 100644
> > --- a/ndctl/lib/private.h
> > +++ b/ndctl/lib/private.h
> > @@ -129,6 +129,7 @@ struct ndctl_ctx {
> > int regions_init;
> > void *userdata;
> > struct list_head busses;
> > + const char *configs;
> > int busses_init;
> > struct udev *udev;
> > struct udev_queue *udev_queue;
> > diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
> > index 536e142..6b68e3a 100644
> > --- a/ndctl/lib/libndctl.c
> > +++ b/ndctl/lib/libndctl.c
> > @@ -24,6 +24,7 @@
> > #include <util/util.h>
> > #include <util/size.h>
> > #include <util/sysfs.h>
> > +#include <util/strbuf.h>
> > #include <ndctl/libndctl.h>
> > #include <ndctl/namespace.h>
> > #include <daxctl/libdaxctl.h>
> > @@ -265,6 +266,58 @@ 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, '.');
>
> Oh, I guess clang-format does not insert line breaks between
> declarations and code.
Oh, I'd fixed this, but I guess only in my future patch that moves this
function elsewhere. Since I'm modifying Qi's patches anyway now, I'll
squash that move patch into this, and make sure that fix is picked up
too.
>
> > + if ((!ext) || (ext == dir->d_name))
> > + return 0;
> > + if (strcmp(ext, ".conf") == 0)
> > + return 1;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +NDCTL_EXPORT void ndctl_set_configs_dir(struct ndctl_ctx **ctx, char
> > *conf_dir)
>
> ndctl_set_config_path
Sounds good.
>
> Why is ctx a 'struct ndctl_ctx **' instead of a 'struct ndctl_ctx *'?
Heh I'd noticed this too but forgot to fix it, will do now.
>
> > +{
> > + struct dirent **namelist;
> > + struct strbuf value = STRBUF_INIT;
> > + int rc;
> > +
> > + if ((!ctx) || (!conf_dir))
>
> I think this function needs to return an error. If someone does:
> ndctl_set_config_path(NULL, "/path") nothing will notify them that
> /path is not established as the config directory. The ENOENT check is
> helpful, but why not tell the caller?
Yes agreed, I'll change this.
>
> > + return;
> > +
> > + rc = scandir(conf_dir, &namelist, filter_conf, alphasort);
> > + if (rc == -1) {
> > + if (errno != ENOENT)
> > + err(*ctx, "scandir for configs failed: %s\n",
> > + strerror(errno));
> > + 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]);
> > + }
>
> Any reason this needs to be converted to a space separated list? What
> if there are spaces in the path name?
>
> This could just cache the scandir result in the ctx directly and skip
> the strbuf usage.
Hm I hadn't looked too closely at this before, but makes sense, and
good point on paths with spaces. I'll fix.
>
> > + (*ctx)->configs = strbuf_detach(&value, NULL);
> > + free(namelist);
> > +}
> > +
> > +NDCTL_EXPORT const char *ndctl_get_configs_dir(struct ndctl_ctx *ctx)
> > +{
> > + if (ctx == NULL)
> > + return NULL;
> > + return ctx->configs;
> > +}
> > +
> > /**
> > * ndctl_new - instantiate a new library context
> > * @ctx: context to establish
> > @@ -331,6 +384,7 @@ NDCTL_EXPORT int ndctl_new(struct ndctl_ctx **ctx)
> > c->daxctl_ctx = daxctl_ctx;
> >
> > return 0;
> > +
>
> Unnecessary newline.
>
> > err_ctx:
> > daxctl_unref(daxctl_ctx);
> > err_daxctl:
> > diff --git a/ndctl/libndctl.h b/ndctl/libndctl.h
> > index 87d07b7..883a56f 100644
> > --- a/ndctl/libndctl.h
> > +++ b/ndctl/libndctl.h
> > @@ -92,6 +92,8 @@ int ndctl_get_log_priority(struct ndctl_ctx *ctx);
> > void ndctl_set_log_priority(struct ndctl_ctx *ctx, int priority);
> > void ndctl_set_userdata(struct ndctl_ctx *ctx, void *userdata);
> > void *ndctl_get_userdata(struct ndctl_ctx *ctx);
> > +void ndctl_set_configs_dir(struct ndctl_ctx **ctx, char *conf_dir);
> > +const char *ndctl_get_configs_dir(struct ndctl_ctx *ctx);
> >
> > enum ndctl_persistence_domain {
> > PERSISTENCE_NONE = 0,
> > diff --git a/ndctl/ndctl.c b/ndctl/ndctl.c
> > index 31d2c5e..0f908db 100644
> > --- a/ndctl/ndctl.c
> > +++ b/ndctl/ndctl.c
> > @@ -125,6 +125,7 @@ int main(int argc, const char **argv)
> > rc = ndctl_new(&ctx);
> > if (rc)
> > goto out;
> > + ndctl_set_configs_dir(&ctx, NDCTL_CONF_DIR);
>
> I expect this to happen by default in ndctl_new().
>
> In fact, ndctl_new() might want to grow a config path argument to
> override the default.
Happening by default in ndctl_new sounds good.
However since ndctl_new() is an exported API, we can't just add a new
argument to it, can we? Do we expect callers to want custom paths,
outside of what a distro may set via NDCTL_CONF_DIR during build?
>
> > main_handle_internal_command(argc, argv, ctx, commands,
> > ARRAY_SIZE(commands), PROG_NDCTL);
> > ndctl_unref(ctx);
> > diff --git a/ndctl/Makefile.am b/ndctl/Makefile.am
> > index a63b1e0..1caa031 100644
> > --- a/ndctl/Makefile.am
> > +++ b/ndctl/Makefile.am
> > @@ -7,8 +7,9 @@ BUILT_SOURCES = config.h
> > config.h: $(srcdir)/Makefile.am
> > $(AM_V_GEN) echo "/* Autogenerated by ndctl/Makefile.am */" >$@ && \
> > echo '#define NDCTL_CONF_FILE \
> > - "$(ndctl_monitorconfdir)/$(ndctl_monitorconf)"' >>$@
> > + "$(ndctl_confdir)/$(ndctl_monitorconf)"' >>$@
> > $(AM_V_GEN) echo '#define NDCTL_KEYS_DIR "$(ndctl_keysdir)"' >>$@
> > + $(AM_V_GEN) echo '#define NDCTL_CONF_DIR "$(ndctl_confdir)"' >>$@
> >
> > ndctl_SOURCES = ndctl.c \
> > builtin.h \
> > @@ -73,7 +74,7 @@ ndctl_SOURCES += ../test/libndctl.c \
> > test.c
> > endif
> >
> > -monitor_configdir = $(ndctl_monitorconfdir)
> > +monitor_configdir = $(ndctl_confdir)
> > monitor_config_DATA = $(ndctl_monitorconf)
> >
> > if ENABLE_SYSTEMD_UNITS
> > diff --git a/ndctl/lib/Makefile.am b/ndctl/lib/Makefile.am
> > index e15bb22..f741c44 100644
> > --- a/ndctl/lib/Makefile.am
> > +++ b/ndctl/lib/Makefile.am
> > @@ -14,6 +14,10 @@ libndctl_la_SOURCES =\
> > ../../util/log.h \
> > ../../util/sysfs.c \
> > ../../util/sysfs.h \
> > + ../../util/strbuf.h \
> > + ../../util/strbuf.c \
> > + ../../util/wrapper.c \
> > + ../../util/usage.c \
>
> Seems this new utility code linking is not necessary per the above
> recommendation to just cache the scandir results in the ctx.
Yep makes sense.
>
> > ../../util/fletcher.h \
> > dimm.c \
> > inject.c \
> > diff --git a/ndctl/lib/libndctl.sym b/ndctl/lib/libndctl.sym
> > index 58afb74..6e1b510 100644
> > --- a/ndctl/lib/libndctl.sym
> > +++ b/ndctl/lib/libndctl.sym
> > @@ -454,4 +454,6 @@ LIBNDCTL_25 {
> >
> > LIBNDCTL_26 {
> > ndctl_bus_nfit_translate_spa;
> > + ndctl_set_configs_dir;
> > + ndctl_get_configs_dir;
> > } LIBNDCTL_25;
> > --
> > 2.33.1
> >
