Re: [PATCH v2] Add kernel-version dependent configuration directory

["Anton V. Boyarshinov" <boyarsh@altlinux.org>]

> 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