> 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
