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

[Dave Reisner]

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