linux-modules.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] Add kernel-version dependent configuration directory
@ 2020-02-26 13:32 Anton V. Boyarshinov
  2020-02-27  8:43 ` Lucas De Marchi
  0 siblings, 1 reply; 4+ messages in thread
From: Anton V. Boyarshinov @ 2020-02-26 13:32 UTC (permalink / raw)
  To: linux-modules; +Cc: legion

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`

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);
+		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



^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] Add kernel-version dependent configuration directory
  2020-02-26 13:32 [PATCH v2] Add kernel-version dependent configuration directory Anton V. Boyarshinov
@ 2020-02-27  8:43 ` Lucas De Marchi
  2020-02-27 11:09   ` Anton V. Boyarshinov
  2020-02-27 12:41   ` Anton V. Boyarshinov
  0 siblings, 2 replies; 4+ messages in thread
From: Lucas De Marchi @ 2020-02-27  8:43 UTC (permalink / raw)
  To: Anton V. Boyarshinov; +Cc: linux-modules, legion

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] Add kernel-version dependent configuration directory
  2020-02-27  8:43 ` Lucas De Marchi
@ 2020-02-27 11:09   ` Anton V. Boyarshinov
  2020-02-27 12:41   ` Anton V. Boyarshinov
  1 sibling, 0 replies; 4+ messages in thread
From: Anton V. Boyarshinov @ 2020-02-27 11:09 UTC (permalink / raw)
  To: Lucas De Marchi; +Cc: linux-modules, legion


> Thanks for the patch, I really like the idea of supporting this. I
> have some suggestions below.

Thank you for review. 
> >
> > -       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.

Yes, i haven't read it carefully enouth, but it can be easyly fixed by checking
first element.
 
> I wonder if for implementing this it wouldn't be better to leave this
> function alone and implement it
> in kmod_config_new():

But how to distinguish in mod_config_new(): are provided config pointer
default config and needs version-dep paths adding or it is a custom config?
I've considered this options but haven't found a straight way to do this.
 
> 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.

AFAIK in reverse, if dirname is NOT NULL, we a not running for the current kernel.
And you are right, my patch will cause evil effects when not current kernel is 
specified. Not a problem for modprobe, but it is common code.
 
> 2) conf_files_list(): after "opendir(path)" we try a opendirat(d,
> ctx->kver...) and just ignore if it doesn't exist.

But should we try to open versions-dependent dirs in user-specified dirs?
It looks unexpected, considering user-specified dirs semantics.
 
> 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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] Add kernel-version dependent configuration directory
  2020-02-27  8:43 ` Lucas De Marchi
  2020-02-27 11:09   ` Anton V. Boyarshinov
@ 2020-02-27 12:41   ` Anton V. Boyarshinov
  1 sibling, 0 replies; 4+ messages in thread
From: Anton V. Boyarshinov @ 2020-02-27 12:41 UTC (permalink / raw)
  To: Lucas De Marchi; +Cc: linux-modules, legion

On Thu, 27 Feb 2020 00:43:47 -0800 Lucas De Marchi wrote:

> See documentation above this function. This breaks the case in which
> the supplied array is empty,
> i.e. a single NULL element.

I've review this code and disagree with you. It doesn't break
single NULL element vector. In this case config_paths is not NULL and,
so no version-dependent configuation will be created. Single NULL will
be passed as is, like any other custom configuration.

   if (config_paths == NULL) {
	/*creating version-dependent configuration */
                err = kmod_config_new(ctx, &ctx->config, (const char * const*) tmp_config_paths);
	/* free resources */
        }
        else
                err = kmod_config_new(ctx, &ctx->config, config_paths);


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2020-02-27 12:41 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-26 13:32 [PATCH v2] Add kernel-version dependent configuration directory Anton V. Boyarshinov
2020-02-27  8:43 ` Lucas De Marchi
2020-02-27 11:09   ` Anton V. Boyarshinov
2020-02-27 12:41   ` Anton V. Boyarshinov

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