| Message ID | 20200226133221.44342a57@table.localdomain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] Add kernel-version dependent configuration directory | expand |
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 > >
> 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
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);
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>
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(-)