linux-modules.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Anton V. Boyarshinov" <boyarsh@altlinux.org>
To: Lucas De Marchi <lucas.de.marchi@gmail.com>
Cc: linux-modules <linux-modules@vger.kernel.org>, legion@altlinux.org
Subject: Re: [PATCH v2] Add kernel-version dependent configuration directory
Date: Thu, 27 Feb 2020 14:09:15 +0300	[thread overview]
Message-ID: <20200227140915.7fa32b2e@boyarsh.office.basealt.ru> (raw)
In-Reply-To: <CAKi4VA+8o=3HASrtuFNwts35q09xR_bUAY8Bd8_tc0ebj4DZ=g@mail.gmail.com>


> 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

  reply	other threads:[~2020-02-27 11:09 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-26 13:32 Anton V. Boyarshinov
2020-02-27  8:43 ` Lucas De Marchi
2020-02-27 11:09   ` Anton V. Boyarshinov [this message]
2020-02-27 12:41   ` Anton V. Boyarshinov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200227140915.7fa32b2e@boyarsh.office.basealt.ru \
    --to=boyarsh@altlinux.org \
    --cc=legion@altlinux.org \
    --cc=linux-modules@vger.kernel.org \
    --cc=lucas.de.marchi@gmail.com \
    --subject='Re: [PATCH v2] Add kernel-version dependent configuration directory' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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