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

Thank you for review. 

quoted

-       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

quoted

+               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