On Wed, Feb 26, 2020 at 5:32 AM Anton V. Boyarshinov
<boyarsh@xxxxxxxxxxxx> wrote:
>
> In some cases it looks reasonable to have kernel-version dependent
> modules configuration: blacklists, options and so on. This commit
> introduces additional configuration directories:
> * /lib/modprobe.d/`uname -r`
> * /etc/modprobe.d/`uname -r`
Thanks for the patch, I really like the idea of supporting this. I
have some suggestions below.
>
> Signed-off-by: Anton V. Boyarshinov <boyarsh@xxxxxxxxxxxx>
> ---
> Changes from v1: ARRAY_SIZE and SYSCONFDIR macros are used
>
> libkmod/libkmod-config.c | 5 +----
> libkmod/libkmod.c | 44 ++++++++++++++++++++++++++++++++++++----
> man/modprobe.d.xml | 2 ++
> 3 files changed, 43 insertions(+), 8 deletions(-)
>
> diff --git a/libkmod/libkmod-config.c b/libkmod/libkmod-config.c
> index aaac0a1..5cc348a 100644
> --- a/libkmod/libkmod-config.c
> +++ b/libkmod/libkmod-config.c
> @@ -711,11 +711,8 @@ static bool conf_files_filter_out(struct kmod_ctx *ctx, DIR *d,
>
> fstatat(dirfd(d), fn, &st, 0);
>
> - if (S_ISDIR(st.st_mode)) {
> - ERR(ctx, "Directories inside directories are not supported: "
> - "%s/%s\n", path, fn);
> + if (S_ISDIR(st.st_mode))
> return true;
> - }
>
> return false;
> }
> diff --git a/libkmod/libkmod.c b/libkmod/libkmod.c
> index 69fe431..b718da0 100644
> --- a/libkmod/libkmod.c
> +++ b/libkmod/libkmod.c
> @@ -225,6 +225,21 @@ static char *get_kernel_release(const char *dirname)
> return p;
> }
>
> +static char *get_ver_config_path(const char * dir)
> +{
> + struct utsname u;
> + char *ver_conf;
> + static const char appendix[] = "modprobe.d";
> +
> + if (uname(&u) < 0)
> + return NULL;
> +
> + if (asprintf(&ver_conf, "%s/%s/%s", dir, appendix, u.release) < 0)
> + return NULL;
> +
> + return ver_conf;
> +}
> +
> /**
> * kmod_new:
> * @dirname: what to consider as linux module's directory, if NULL
> @@ -251,7 +266,7 @@ KMOD_EXPORT struct kmod_ctx *kmod_new(const char *dirname,
> {
> const char *env;
> struct kmod_ctx *ctx;
> - int err;
> + int err, configs_count;
>
> ctx = calloc(1, sizeof(struct kmod_ctx));
> if (!ctx)
> @@ -269,9 +284,30 @@ KMOD_EXPORT struct kmod_ctx *kmod_new(const char *dirname,
> if (env != NULL)
> kmod_set_log_priority(ctx, log_priority(env));
>
> - if (config_paths == NULL)
> - config_paths = default_config_paths;
> - err = kmod_config_new(ctx, &ctx->config, config_paths);
> + if (config_paths == NULL) {
> + char **tmp_config_paths = malloc(sizeof(default_config_paths) +
> + sizeof(char *) * 2);
See documentation above this function. This breaks the case in which
the supplied array is empty,
i.e. a single NULL element.
I wonder if for implementing this it wouldn't be better to leave this
function alone and implement it
in kmod_config_new():
1) we create ctx->kver that points to the end of ctx->dirname. If
dirname is NULL in kmod_new(), then
it's assumed we are actually not running for the current kernel, so we
might leave this logic alone.
2) conf_files_list(): after "opendir(path)" we try a opendirat(d,
ctx->kver...) and just ignore if it doesn't exist.
then we run the rest of the logic as usual.
This should ensure that a) we don't break the API, b) we honor dirname
== NULL meaning "don't treat this ctx as
one for the currently running kernel" and also c) we allow
kernel-version subdirs for a user-provided list.
What do you think?
Lucas De Marchi
> + if(tmp_config_paths == NULL) {
> + ERR(ctx, "could not create versioned config.\n");
> + goto fail;
> + }
> +
> + memcpy(tmp_config_paths, default_config_paths, sizeof(default_config_paths));
> +
> + configs_count = ARRAY_SIZE(default_config_paths);
> + tmp_config_paths[configs_count-1] = get_ver_config_path("/lib");
> + tmp_config_paths[configs_count] = get_ver_config_path(SYSCONFDIR);
> + tmp_config_paths[configs_count+1] = NULL;
> +
> + err = kmod_config_new(ctx, &ctx->config, (const char * const*) tmp_config_paths);
> +
> + free(tmp_config_paths[configs_count-1]);
> + free(tmp_config_paths[configs_count]);
> + free(tmp_config_paths);
> + }
> + else
> + err = kmod_config_new(ctx, &ctx->config, config_paths);
> +
> if (err < 0) {
> ERR(ctx, "could not create config\n");
> goto fail;
> diff --git a/man/modprobe.d.xml b/man/modprobe.d.xml
> index 211af84..d1e254a 100644
> --- a/man/modprobe.d.xml
> +++ b/man/modprobe.d.xml
> @@ -41,7 +41,9 @@
>
> <refsynopsisdiv>
> <para><filename>/lib/modprobe.d/*.conf</filename></para>
> + <para><filename>/lib/modprobe.d/`uname -r`/*.conf</filename></para>
> <para><filename>/etc/modprobe.d/*.conf</filename></para>
> + <para><filename>/etc/modprobe.d/`uname -r`/*.conf</filename></para>
> <para><filename>/run/modprobe.d/*.conf</filename></para>
> </refsynopsisdiv>
>
> --
> 2.21.0
>
>
--
Lucas De Marchi
