On Wed, Aug 31, 2011 at 11:39:16AM +0200, Sebastien Luttringer wrote:

This patch implement loading of sysctl config files as described in http://0pointer.de/public/systemd-man/sysctl.d.html

This is a very conveniant way of configuring sysctl option for sysadmin which can drop sysctl config files inside this directory to enable some feature.

Dropping a file like disableipv6.conf inside this directory will disable ipv6 $ cat disableipv6.conf net.ipv6.conf.all.disable_ipv6 = 1

There is atm no package which use this functionnality

NOTE: /etc/sysctl.d/ and /usr/lib/sysctl.d/ should be added in procps

procps wants no part of this. It only cares about /etc/sysctl.conf. Add these directories to the Makefile here, please.

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.

--- rc.multi | 22 +++++++++++++++++++++- tmpfiles.conf | 1 + 2 files changed, 22 insertions(+), 1 deletions(-)

diff --git a/rc.multi b/rc.multi index 16fa83a..5974a35 100755 --- a/rc.multi +++ b/rc.multi @@ -6,11 +6,31 @@ . /etc/rc.conf . /etc/rc.d/functions

+shopt -s nullglob + run_hook multi_start

-# Load sysctl variables if sysctl.conf is present +# Load sysctl variables from sysctl.conf (backward compatibility) [[ -r /etc/sysctl.conf ]] && sysctl -q -p &>/dev/null

+# 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.

+declare -A sysctl_c +for _d in "${sysctl_d[@]}"; do + [[ -d "$_d" ]] || continue + for _f in "$_d"/*.conf; do + sysctl_c[${_f##*/}]="$_f" + done +done +for _f in "${sysctl_c[@]}"; do + sysctl -q -p "$_f" &>/dev/null

We've already uniq'd the array, so this should't require multiple invocations: (( ${#fragments[*]} )) && cat "${fragments[@]}" | sysctl -q -p -

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

d /tmp/.X11-unix 1777 root root d /tmp/.ICE-unix 1777 root root --

General: There's no need to make your variable names so cryptic. 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). d

On Wed, Aug 31, 2011 at 01:02:11PM +0200, Seblu wrote:

On Wed, Aug 31, 2011 at 12:19 PM, Dave Reisner 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.