[arch-projects] [INITSCRIPTS][PATCH] Load sysctl config files from sysctl.d

[Dave Reisner]

On Wed, Aug 31, 2011 at 01:02:11PM +0200, Seblu wrote:
> On Wed, Aug 31, 2011 at 12:19 PM, Dave Reisner <d at falconindy.com> wrote:
> >> NOTE: maybe this feature should be added as a daemon for procps package
> >> allowing user to reload configs files by calling rc.d reload procps
> >
> > What's the "off" version of this setting:
> >
> > vm.swappiness=0
> >
> > I have no idea how you intend to "turn off" sysctl setting as part of a
> > "restart", nor can I figure out why this might be needed.
> I doesn't want turn off a feature. Just reload the config files.
> Before we do this by calling sysctl -p.
> Now we have more than one file to load and sysctl -p doesn't make the jobs.
> So an helper which reload all files is handy. Debian have this "feature".

Seems like a fix for something that's awkwardly broken elsewhere. If
this is split off to its own script, a user can "reload" this at their
leisure.

> >> +# Load sysctl variables (new style)
> >> +# http://0pointer.de/public/systemd-man/sysctl.d.html
> >> +declare -a sysctl_d=(
> >> +     /usr/lib/sysctl.d
> >> +     /etc/sysctl.d
> >> +     /run/sysctl.d
> >> +)
> >
> > You enabled nullglob, so if you declare this array as globs of *.conf
> > files, none of the directory checking is necessary and we get a singular
> > loop over an array of files. It'd be nice to actually check that what
> > we're adding is a regular file, though, as opposed to something insane
> > like a fifo sneaking in.
> ok i will add this check.
> About all inside sysctl_d, we need (as spec require it), to order file
> in this directory order an overload them if there are the same name.
> If we do like this it will be difficult to reorder correctly after.

I'm aware of the spec. Globs expand in alphapbetical order (sorted
according to the C locale) but the directory order will be preserved.
With nullglob enabled, we get the directory existance check for free.
This is exactly what we do in arch-tmpfiles and it works quite well.

> About nullglob i'm not sure it's the good way. I would you experience
> about this, because it can cause cross damage.

Sorry, I have no idea what this means.

> 
> > We've already uniq'd the array, so this should't require multiple
> > invocations:
> >
> > (( ${#fragments[*]} )) && cat "${fragments[@]}" | sysctl -q -p -
> the first load all valid files and doesn't stop at first error and
> show a pretty message
> warning: /etc/sysctl.d/toto toto.conf(1): invalid syntax, continuing...

I don't really follow what you're trying to say here either.

 $ sysctl -p -
 this.isnt.a.real.key=3
 error: "this.isnt.a.real.key" is an unknown key
 vm.swappiness=0
 error: permission denied on key 'vm.swappiness'
 bogus
 warning: -(3): invalid syntax, continuing...

We only get a useful warning (tying it back to a file) on bad syntax and
never for permission denied (unlikely) or invalid keys. This is garbage,
really. I, personally, don't think this is worth preserving in any way
so we may as well avoid the needless forking. If we wanted to take the
time to get proper error messages, we'd need to traverse /proc/sys
ourselves (or patch procps).

> I should remove to have a correct ouput on error &>/dev/null
> 
> >
> >> +done
> >>  # Start daemons
> >>  for daemon in "${DAEMONS[@]}"; do
> >>       case ${daemon:0:1} in
> >> diff --git a/tmpfiles.conf b/tmpfiles.conf
> >> index 7dd1358..61081d0 100644
> >> --- a/tmpfiles.conf
> >> +++ b/tmpfiles.conf
> >> @@ -4,6 +4,7 @@
> >>
> >>  D /tmp              1777 root root
> >>  D /run/daemons      0755 root root
> >> +D /run/sysctl.d     0755 root root
> >
> > By the time this runs, it's too late to care about creating this
> > directory. Further, if /run/sysctl.d does exist and someone dropped a
> > .conf in it, you just wiped it out, as D will clean out an existing
> > directory.
> ok
> 
> > General:
> > There's no need to make your variable names so cryptic.
> ok
> 
> > I'm also not sure why this needs to be embedded into /etc/rc.multi as opposed to
> > living in its own script or a function (where you could namespace
> > properly).
> I agree with this. This is no race condition on this loading which
> need it before running the daemons.
> 
> I think the better way is to add a daemon in propcs package with
> directories. Because this package own sysctl and sysctl.conf. Why
> scatter more than necessary?
> Find a good place to create /run/sysctl.d
> Add in default rc.conf,  procps as first lauched daemon (procps is in
> the base group)
> 

It's about not adding random scripts to other packages, even as dead as
procps is upstream. We didn't add arch-tmpfiles to the filesystem
package. And please, no more "daemons" who don't actually have long
running processes.

d

P.S. I'm still not really sold on the need for this.