linux-modules.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Lucas De Marchi <lucas.de.marchi@gmail.com>
To: "Anton V. Boyarshinov" <boyarsh@altlinux.org>
Cc: linux-modules <linux-modules@vger.kernel.org>, legion@altlinux.org
Subject: Re: [PATCH v2] Add kernel-version dependent configuration directory
Date: Thu, 27 Feb 2020 00:43:47 -0800	[thread overview]
Message-ID: <CAKi4VA+8o=3HASrtuFNwts35q09xR_bUAY8Bd8_tc0ebj4DZ=g@mail.gmail.com> (raw)
In-Reply-To: <20200226133221.44342a57@table.localdomain>

On Wed, Feb 26, 2020 at 5:32 AM Anton V. Boyarshinov
<boyarsh@altlinux.org> 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@altlinux.org>
> ---
> 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

  reply	other threads:[~2020-02-27  8:44 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-26 13:32 Anton V. Boyarshinov
2020-02-27  8:43 ` Lucas De Marchi [this message]
2020-02-27 11:09   ` Anton V. Boyarshinov
2020-02-27 12:41   ` Anton V. Boyarshinov

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='CAKi4VA+8o=3HASrtuFNwts35q09xR_bUAY8Bd8_tc0ebj4DZ=g@mail.gmail.com' \
    --to=lucas.de.marchi@gmail.com \
    --cc=boyarsh@altlinux.org \
    --cc=legion@altlinux.org \
    --cc=linux-modules@vger.kernel.org \
    --subject='Re: [PATCH v2] Add kernel-version dependent configuration directory' \
    /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

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).