[Buildroot] [PATCH 0/2] Add a sysctl init script

[Buildroot] [PATCH 0/2] Add a sysctl init script

[Carlos Santos]

Add a simple init script that invokes sysctl early in the initialization
process to configure kernel parameters. This is already performed by
systemd (systemd-sysctl) but there is no sysvinit/busybox counterpart.

Files are read from directories in the following list in the given order
from top to bottom:

    /run/sysctl.d/*.conf
    /etc/sysctl.d/*.conf
    /usr/local/lib/sysctl.d/*.conf
    /usr/lib/sysctl.d/*.conf
    /lib/sysctl.d/*.conf
    /etc/sysctl.conf

Carlos Santos (2):
  package/procps-ng: add init script for sysctl
  package/busybox: add init script for sysctl

 package/busybox/S01sysctl      | 63 ++++++++++++++++++++++++++++++++++
 package/busybox/busybox.mk     | 12 +++++++
 package/procps-ng/S01sysctl    | 41 ++++++++++++++++++++++
 package/procps-ng/procps-ng.mk |  5 +++
 4 files changed, 121 insertions(+)
 create mode 100644 package/busybox/S01sysctl
 create mode 100644 package/procps-ng/S01sysctl

-- 
2.19.2

[Buildroot] [PATCH 1/2] package/procps-ng: add init script for sysctl

[Carlos Santos]

Add a simple init script that invokes sysctl early in the initialization
process to configure kernel parameters. This is already performed by
systemd (systemd-sysctl) but there is no sysvinit/busybox counterpart.

Files are read from directories in the following list in the given order
from top to bottom:

    /run/sysctl.d/*.conf
    /etc/sysctl.d/*.conf
    /usr/local/lib/sysctl.d/*.conf
    /usr/lib/sysctl.d/*.conf
    /lib/sysctl.d/*.conf
    /etc/sysctl.conf

Signed-off-by: Carlos Santos <casantos@datacom.com.br>
---
 package/procps-ng/S01sysctl    | 41 ++++++++++++++++++++++++++++++++++
 package/procps-ng/procps-ng.mk |  5 +++++
 2 files changed, 46 insertions(+)
 create mode 100644 package/procps-ng/S01sysctl

diff --git a/package/procps-ng/S01sysctl b/package/procps-ng/S01sysctl
new file mode 100644
index 0000000000..e93bdb9e17
--- /dev/null
+++ b/package/procps-ng/S01sysctl
@@ -0,0 +1,41 @@
+#!/bin/sh
+
+PROGRAM="sysctl"
+
+SYSCTL_ARGS=""
+
+# shellcheck source=/dev/null
+[ -r "/etc/default/$PROGRAM" ] && . "/etc/default/$PROGRAM"
+
+start() {
+	printf 'Starting %s: ' "$PROGRAM"
+	# shellcheck disable=SC2086 # we need the word splitting
+	/sbin/sysctl --quiet --system $SYSCTL_ARGS
+	status=$?
+	if [ "$status" -eq 0 ]; then
+		echo "OK"
+	else
+		echo "FAIL"
+	fi
+	return "$status"
+}
+
+stop() {
+	printf 'Stopping %s: OK\n' "$PROGRAM"
+}
+
+restart() {
+	stop
+	sleep 1
+	start
+}
+
+case "$1" in
+        start|stop)
+		"$1";;
+	restart|reload)
+		restart;;
+        *)
+                echo "Usage: $0 {start|stop|restart|reload}"
+                exit 1
+esac
diff --git a/package/procps-ng/procps-ng.mk b/package/procps-ng/procps-ng.mk
index 03b74784d2..e40aeb5a2a 100644
--- a/package/procps-ng/procps-ng.mk
+++ b/package/procps-ng/procps-ng.mk
@@ -44,4 +44,9 @@ ifeq ($(BR2_STATIC_LIBS),y)
 PROCPS_NG_CONF_OPTS += --disable-numa
 endif
 
+define PROCPS_NG_INSTALL_INIT_SYSV
+	$(INSTALL) -D -m 755 package/procps-ng/S01sysctl \
+		$(TARGET_DIR)/etc/init.d/S01sysctl
+endef
+
 $(eval $(autotools-package))
-- 
2.19.2

[Buildroot] [PATCH 2/2] package/busybox: add init script for sysctl

[Carlos Santos]

Add a simple init script that invokes sysctl early in the initialization
process to configure kernel parameters. This is already performed by
systemd (systemd-sysctl) but there is no sysvinit/busybox counterpart.

Files are read from directories in the following list in the given order
from top to bottom:

    /run/sysctl.d/*.conf
    /etc/sysctl.d/*.conf
    /usr/local/lib/sysctl.d/*.conf
    /usr/lib/sysctl.d/*.conf
    /lib/sysctl.d/*.conf
    /etc/sysctl.conf

A file may be used more than once, since there can be multiple symlinks
to it. No attempt is made to prevent this.

Busybox's sysctl does not support "--quiet --system" arguments like the
sysctl provided by procps-ng, so we resort to some scripting to mimic
its behavior.

Signed-off-by: Carlos Santos <casantos@datacom.com.br>
---
 package/busybox/S01sysctl  | 63 ++++++++++++++++++++++++++++++++++++++
 package/busybox/busybox.mk | 12 ++++++++
 2 files changed, 75 insertions(+)
 create mode 100644 package/busybox/S01sysctl

diff --git a/package/busybox/S01sysctl b/package/busybox/S01sysctl
new file mode 100644
index 0000000000..35e0cef291
--- /dev/null
+++ b/package/busybox/S01sysctl
@@ -0,0 +1,63 @@
+#!/bin/sh
+
+PROGRAM="sysctl"
+
+SYSCTL_ARGS=""
+
+# shellcheck source=/dev/null
+[ -r "/etc/default/$PROGRAM" ] && . "/etc/default/$PROGRAM"
+
+# Files are read from directories in the SYSCTL_SOURCES list, in given order.
+SYSCTL_SOURCES="/etc/sysctl.d/ /usr/local/lib/sysctl.d/ /usr/lib/sysctl.d/ /lib/sysctl.d/ /etc/sysctl.conf"
+
+# A file may be used more than once, since there can be multiple symlinks to
+# it. No attempt is made to prevent this.
+#
+# Busybox's sysctl does not support "--quiet --system" arguments like the
+# sysctl provided by procps-ng, so we resort to some scripting to mimic its
+# behavior.
+#
+# The "return 1" below is fruitless, at the moment, since sysctl ends with
+# status zero even if errors happen. Hopefully this will be fixed in a future
+# version of Busybox.
+apply_configs() {
+	# shellcheck disable=SC2086 # we need the word splitting
+	find $SYSCTL_SOURCES -maxdepth 1 -name '*.conf' -print0 2> /dev/null | \
+	xargs -0 -r readlink -f | \
+	{
+		read -r file
+		[ -z "$file" ] || /sbin/sysctl -q -p "$file" $SYSCTL_ARGS || return 1
+	}
+}
+
+start() {
+	printf 'Starting %s: ' "$PROGRAM"
+	apply_configs
+	status=$?
+	if [ "$status" -eq 0 ]; then
+		echo "OK"
+	else
+		echo "FAIL"
+	fi
+	return "$status"
+}
+
+stop() {
+	printf 'Stopping %s: OK\n' "$PROGRAM"
+}
+
+restart() {
+	stop
+	sleep 1
+	start
+}
+
+case "$1" in
+        start|stop)
+		"$1";;
+	restart|reload)
+		restart;;
+        *)
+                echo "Usage: $0 {start|stop|restart|reload}"
+                exit 1
+esac
diff --git a/package/busybox/busybox.mk b/package/busybox/busybox.mk
index bfcca6ed3e..bf101d7d46 100644
--- a/package/busybox/busybox.mk
+++ b/package/busybox/busybox.mk
@@ -259,6 +259,17 @@ define BUSYBOX_INSTALL_LOGGING_SCRIPT
 endef
 endif
 
+# Only install our sysctl scripts if no other package does it.
+ifeq ($(BR2_PACKAGE_PROCPS_NG),)
+define BUSYBOX_INSTALL_SYSCTL_SCRIPT
+	if grep -q CONFIG_BB_SYSCTL=y $(@D)/.config; \
+	then \
+		$(INSTALL) -m 0755 -D package/busybox/S01sysctl \
+			$(TARGET_DIR)/etc/init.d/S01sysctl ; \
+	fi
+endef
+endif
+
 ifeq ($(BR2_INIT_BUSYBOX),y)
 define BUSYBOX_INSTALL_INITTAB
 	$(INSTALL) -D -m 0644 package/busybox/inittab $(TARGET_DIR)/etc/inittab
@@ -344,6 +355,7 @@ define BUSYBOX_INSTALL_INIT_SYSV
 	$(BUSYBOX_INSTALL_MDEV_SCRIPT)
 	$(BUSYBOX_INSTALL_LOGGING_SCRIPT)
 	$(BUSYBOX_INSTALL_WATCHDOG_SCRIPT)
+	$(BUSYBOX_INSTALL_SYSCTL_SCRIPT)
 	$(BUSYBOX_INSTALL_TELNET_SCRIPT)
 	$(BUSYBOX_INSTALL_INDIVIDUAL_BINARIES)
 endef
-- 
2.19.2

[Buildroot] [PATCH 2/2] package/busybox: add init script for sysctl

[Matthew Weber]

Carlos,



On Mon, Dec 17, 2018 at 11:03 PM Carlos Santos <casantos@datacom.com.br> wrote:
>
> Add a simple init script that invokes sysctl early in the initialization
> process to configure kernel parameters. This is already performed by
> systemd (systemd-sysctl) but there is no sysvinit/busybox counterpart.
>
> Files are read from directories in the following list in the given order
> from top to bottom:
>
>     /run/sysctl.d/*.conf
>     /etc/sysctl.d/*.conf
>     /usr/local/lib/sysctl.d/*.conf
>     /usr/lib/sysctl.d/*.conf
>     /lib/sysctl.d/*.conf
>     /etc/sysctl.conf
>
> A file may be used more than once, since there can be multiple symlinks
> to it. No attempt is made to prevent this.
>
> Busybox's sysctl does not support "--quiet --system" arguments like the
> sysctl provided by procps-ng, so we resort to some scripting to mimic
> its behavior.
>
> Signed-off-by: Carlos Santos <casantos@datacom.com.br>

Thanks for sending this one in.  We usually have some version of a
script that does similar in each product rootfs overlay.

I've reviewed the general function of the script is correct, I did not
verify the SYSCTL_SOURCES are all the right plans to include.  We
primarily just use the /etc/sysctl.conf default config and
/etc/sysctl.d/.

Reviewed-by: Matthew Weber <matthew.weber@rockwellcollins.com>

> ---
>  package/busybox/S01sysctl  | 63 ++++++++++++++++++++++++++++++++++++++
>  package/busybox/busybox.mk | 12 ++++++++
>  2 files changed, 75 insertions(+)
>  create mode 100644 package/busybox/S01sysctl
>
> diff --git a/package/busybox/S01sysctl b/package/busybox/S01sysctl
> new file mode 100644
> index 0000000000..35e0cef291
> --- /dev/null
> +++ b/package/busybox/S01sysctl
> @@ -0,0 +1,63 @@
> +#!/bin/sh
> +
> +PROGRAM="sysctl"
> +
> +SYSCTL_ARGS=""
> +
> +# shellcheck source=/dev/null
> +[ -r "/etc/default/$PROGRAM" ] && . "/etc/default/$PROGRAM"
> +
> +# Files are read from directories in the SYSCTL_SOURCES list, in given order.
> +SYSCTL_SOURCES="/etc/sysctl.d/ /usr/local/lib/sysctl.d/ /usr/lib/sysctl.d/ /lib/sysctl.d/ /etc/sysctl.conf"
> +
> +# A file may be used more than once, since there can be multiple symlinks to
> +# it. No attempt is made to prevent this.
> +#
> +# Busybox's sysctl does not support "--quiet --system" arguments like the
> +# sysctl provided by procps-ng, so we resort to some scripting to mimic its
> +# behavior.
> +#
> +# The "return 1" below is fruitless, at the moment, since sysctl ends with
> +# status zero even if errors happen. Hopefully this will be fixed in a future
> +# version of Busybox.
> +apply_configs() {
> +       # shellcheck disable=SC2086 # we need the word splitting
> +       find $SYSCTL_SOURCES -maxdepth 1 -name '*.conf' -print0 2> /dev/null | \

Why restrict to .conf?

> +       xargs -0 -r readlink -f | \
> +       {
> +               read -r file
> +               [ -z "$file" ] || /sbin/sysctl -q -p "$file" $SYSCTL_ARGS || return 1
> +       }
> +}
> +
> +start() {
> +       printf 'Starting %s: ' "$PROGRAM"
> +       apply_configs
> +       status=$?
> +       if [ "$status" -eq 0 ]; then
> +               echo "OK"
> +       else
> +               echo "FAIL"
> +       fi
> +       return "$status"
> +}
> +
> +stop() {
> +       printf 'Stopping %s: OK\n' "$PROGRAM"
> +}
> +
> +restart() {
> +       stop
> +       sleep 1
> +       start
> +}
> +
> +case "$1" in
> +        start|stop)
> +               "$1";;
> +       restart|reload)
> +               restart;;
> +        *)
> +                echo "Usage: $0 {start|stop|restart|reload}"
> +                exit 1
> +esac
> diff --git a/package/busybox/busybox.mk b/package/busybox/busybox.mk
> index bfcca6ed3e..bf101d7d46 100644
> --- a/package/busybox/busybox.mk
> +++ b/package/busybox/busybox.mk
> @@ -259,6 +259,17 @@ define BUSYBOX_INSTALL_LOGGING_SCRIPT
>  endef
>  endif
>
> +# Only install our sysctl scripts if no other package does it.
> +ifeq ($(BR2_PACKAGE_PROCPS_NG),)
> +define BUSYBOX_INSTALL_SYSCTL_SCRIPT
> +       if grep -q CONFIG_BB_SYSCTL=y $(@D)/.config; \
> +       then \
> +               $(INSTALL) -m 0755 -D package/busybox/S01sysctl \
> +                       $(TARGET_DIR)/etc/init.d/S01sysctl ; \
> +       fi
> +endef
> +endif
> +
>  ifeq ($(BR2_INIT_BUSYBOX),y)
>  define BUSYBOX_INSTALL_INITTAB
>         $(INSTALL) -D -m 0644 package/busybox/inittab $(TARGET_DIR)/etc/inittab
> @@ -344,6 +355,7 @@ define BUSYBOX_INSTALL_INIT_SYSV
>         $(BUSYBOX_INSTALL_MDEV_SCRIPT)
>         $(BUSYBOX_INSTALL_LOGGING_SCRIPT)
>         $(BUSYBOX_INSTALL_WATCHDOG_SCRIPT)
> +       $(BUSYBOX_INSTALL_SYSCTL_SCRIPT)
>         $(BUSYBOX_INSTALL_TELNET_SCRIPT)
>         $(BUSYBOX_INSTALL_INDIVIDUAL_BINARIES)
>  endef
> --
> 2.19.2
>
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot



-- 

Matthew Weber | Pr. Software Engineer | Commercial Avionics

COLLINS AEROSPACE

400 Collins Road NE, Cedar Rapids, Iowa 52498, USA

Tel: +1 319 295 7349 | FAX: +1 319 263 6099

matthew.weber at collins.com | collinsaerospace.com



CONFIDENTIALITY WARNING: This message may contain proprietary and/or
privileged information of Collins Aerospace and its affiliated
companies. If you are not the intended recipient, please 1) Do not
disclose, copy, distribute or use this message or its contents. 2)
Advise the sender by return email. 3) Delete all copies (including all
attachments) from your computer. Your cooperation is greatly
appreciated.

[Buildroot] [PATCH 2/2] package/busybox: add init script for sysctl

[Carlos Santos]

> From: "Matthew Weber" <Matthew.Weber@collins.com>
> To: "Carlos Santos" <casantos@datacom.com.br>
> Cc: "buildroot" <buildroot@buildroot.org>, "ratbert90" <aduskett@gmail.com>, "DATACOM" <rodrigo.rebello@datacom.com.br>
> Sent: Ter?a-feira, 18 de dezembro de 2018 11:49:11
> Subject: Re: [Buildroot] [PATCH 2/2] package/busybox: add init script for sysctl

> Carlos,
> 
> 
> 
> On Mon, Dec 17, 2018 at 11:03 PM Carlos Santos <casantos@datacom.com.br> wrote:
>>
>> Add a simple init script that invokes sysctl early in the initialization
>> process to configure kernel parameters. This is already performed by
>> systemd (systemd-sysctl) but there is no sysvinit/busybox counterpart.
>>
>> Files are read from directories in the following list in the given order
>> from top to bottom:
>>
>>     /run/sysctl.d/*.conf
>>     /etc/sysctl.d/*.conf
>>     /usr/local/lib/sysctl.d/*.conf
>>     /usr/lib/sysctl.d/*.conf
>>     /lib/sysctl.d/*.conf
>>     /etc/sysctl.conf
>>
>> A file may be used more than once, since there can be multiple symlinks
>> to it. No attempt is made to prevent this.
>>
>> Busybox's sysctl does not support "--quiet --system" arguments like the
>> sysctl provided by procps-ng, so we resort to some scripting to mimic
>> its behavior.
>>
>> Signed-off-by: Carlos Santos <casantos@datacom.com.br>
> 
> Thanks for sending this one in.  We usually have some version of a
> script that does similar in each product rootfs overlay.

Same case here.

> I've reviewed the general function of the script is correct, I did not
> verify the SYSCTL_SOURCES are all the right plans to include.  We
> primarily just use the /etc/sysctl.conf default config and
> /etc/sysctl.d/.
> 
> Reviewed-by: Matthew Weber <matthew.weber@rockwellcollins.com>
> 
>> ---
>>  package/busybox/S01sysctl  | 63 ++++++++++++++++++++++++++++++++++++++
>>  package/busybox/busybox.mk | 12 ++++++++
>>  2 files changed, 75 insertions(+)
>>  create mode 100644 package/busybox/S01sysctl
>>
>> diff --git a/package/busybox/S01sysctl b/package/busybox/S01sysctl
>> new file mode 100644
>> index 0000000000..35e0cef291
>> --- /dev/null
>> +++ b/package/busybox/S01sysctl
>> @@ -0,0 +1,63 @@
>> +#!/bin/sh
>> +
>> +PROGRAM="sysctl"
>> +
>> +SYSCTL_ARGS=""
>> +
>> +# shellcheck source=/dev/null
>> +[ -r "/etc/default/$PROGRAM" ] && . "/etc/default/$PROGRAM"
>> +
>> +# Files are read from directories in the SYSCTL_SOURCES list, in given order.
>> +SYSCTL_SOURCES="/etc/sysctl.d/ /usr/local/lib/sysctl.d/ /usr/lib/sysctl.d/
>> /lib/sysctl.d/ /etc/sysctl.conf"
>> +
>> +# A file may be used more than once, since there can be multiple symlinks to
>> +# it. No attempt is made to prevent this.
>> +#
>> +# Busybox's sysctl does not support "--quiet --system" arguments like the
>> +# sysctl provided by procps-ng, so we resort to some scripting to mimic its
>> +# behavior.
>> +#
>> +# The "return 1" below is fruitless, at the moment, since sysctl ends with
>> +# status zero even if errors happen. Hopefully this will be fixed in a future
>> +# version of Busybox.
>> +apply_configs() {
>> +       # shellcheck disable=SC2086 # we need the word splitting
>> +       find $SYSCTL_SOURCES -maxdepth 1 -name '*.conf' -print0 2> /dev/null | \
> 
> Why restrict to .conf?

That's what procps-ng's sysctl does and I wanted to mimic it as
faithfully as possible. It also prevents attempting to use some README
file.

[...]

-- 
Carlos Santos (Casantos) - DATACOM, P&D

[Buildroot] [PATCH 1/2] package/procps-ng: add init script for sysctl

[Arnout Vandecappelle]

 Hi Carlos,

 Since you're the init script master :-) let's do some bikeshedding!

On 18/12/2018 06:02, Carlos Santos wrote:
> Add a simple init script that invokes sysctl early in the initialization
> process to configure kernel parameters. This is already performed by
> systemd (systemd-sysctl) but there is no sysvinit/busybox counterpart.
> 
> Files are read from directories in the following list in the given order
> from top to bottom:
> 
>     /run/sysctl.d/*.conf
>     /etc/sysctl.d/*.conf
>     /usr/local/lib/sysctl.d/*.conf
>     /usr/lib/sysctl.d/*.conf
>     /lib/sysctl.d/*.conf
>     /etc/sysctl.conf
> 
> Signed-off-by: Carlos Santos <casantos@datacom.com.br>
> ---
>  package/procps-ng/S01sysctl    | 41 ++++++++++++++++++++++++++++++++++
>  package/procps-ng/procps-ng.mk |  5 +++++
>  2 files changed, 46 insertions(+)
>  create mode 100644 package/procps-ng/S01sysctl
> 
> diff --git a/package/procps-ng/S01sysctl b/package/procps-ng/S01sysctl
> new file mode 100644
> index 0000000000..e93bdb9e17
> --- /dev/null
> +++ b/package/procps-ng/S01sysctl

 Is 01 the most appropriate place? I would tend to think that we want to log any
errors from sysctl, so it should be 02 or 03.

> @@ -0,0 +1,41 @@
> +#!/bin/sh
> +
> +PROGRAM="sysctl"

 In other init scripts you used DAEMON. Of course, in this case, it's not a
daemon, but maybe for consistency it's better to keep the same?

> +
> +SYSCTL_ARGS=""
> +
> +# shellcheck source=/dev/null
> +[ -r "/etc/default/$PROGRAM" ] && . "/etc/default/$PROGRAM"
> +
> +start() {
> +	printf 'Starting %s: ' "$PROGRAM"
> +	# shellcheck disable=SC2086 # we need the word splitting
> +	/sbin/sysctl --quiet --system $SYSCTL_ARGS

 I'm not so sure about the --quiet here. Even more, I would redirect the output
into logger.

> +	status=$?
> +	if [ "$status" -eq 0 ]; then
> +		echo "OK"
> +	else
> +		echo "FAIL"
> +	fi
> +	return "$status"
> +}
> +
> +stop() {
> +	printf 'Stopping %s: OK\n' "$PROGRAM"

 Since nothing is actually done, I think it's better to not print anything.

> +}
> +
> +restart() {
> +	stop
> +	sleep 1

 And here I'd also skip the stop and more importantly the sleep.

> +	start
> +}
> +
> +case "$1" in
> +        start|stop)

 You put spaces here.

> +		"$1";;
> +	restart|reload)
> +		restart;;

 So here, I'd directly go for 'start' instead of restart. So maybe

case "$1" in
	start|restart|reload)
		start;;
	stop)
		:;;

 I'm not sure, what do you think? Is it better to keep the pattern for the case
statement always the same (except for the restart|reload part)?

 If the pattern should stay the same, in the other scripts you actually put:

        start|stop|restart)
                "$1";;
        reload)
                # Restart, since there is no true "reload" feature.
                restart;;


 Regards,
 Arnout

> +        *)
> +                echo "Usage: $0 {start|stop|restart|reload}"
> +                exit 1
> +esac
> diff --git a/package/procps-ng/procps-ng.mk b/package/procps-ng/procps-ng.mk
> index 03b74784d2..e40aeb5a2a 100644
> --- a/package/procps-ng/procps-ng.mk
> +++ b/package/procps-ng/procps-ng.mk
> @@ -44,4 +44,9 @@ ifeq ($(BR2_STATIC_LIBS),y)
>  PROCPS_NG_CONF_OPTS += --disable-numa
>  endif
>  
> +define PROCPS_NG_INSTALL_INIT_SYSV
> +	$(INSTALL) -D -m 755 package/procps-ng/S01sysctl \
> +		$(TARGET_DIR)/etc/init.d/S01sysctl
> +endef
> +
>  $(eval $(autotools-package))
> 

[Buildroot] [PATCH 2/2] package/busybox: add init script for sysctl

[Nicolas Cavallari]

On 18/12/2018 06:02, Carlos Santos wrote:
> +	find $SYSCTL_SOURCES -maxdepth 1 -name '*.conf' -print0 2> /dev/null | \
> +	xargs -0 -r readlink -f | \
> +	{
> +		read -r file
> +		[ -z "$file" ] || /sbin/sysctl -q -p "$file" $SYSCTL_ARGS || return 1
> +	}

So it will only read the first file it finds ?

[Buildroot] [PATCH 2/2] package/busybox: add init script for sysctl

[Carlos Santos]

> From: "Nicolas Cavallari" <Nicolas.Cavallari@green-communications.fr>
> To: "Carlos Santos" <casantos@datacom.com.br>, "buildroot" <buildroot@buildroot.org>
> Cc: "ratbert90" <aduskett@gmail.com>, "DATACOM" <rodrigo.rebello@datacom.com.br>
> Sent: Ter?a-feira, 18 de dezembro de 2018 14:32:18
> Subject: Re: [Buildroot] [PATCH 2/2] package/busybox: add init script for sysctl

> On 18/12/2018 06:02, Carlos Santos wrote:
>> +	find $SYSCTL_SOURCES -maxdepth 1 -name '*.conf' -print0 2> /dev/null | \
>> +	xargs -0 -r readlink -f | \
>> +	{
>> +		read -r file
>> +		[ -z "$file" ] || /sbin/sysctl -q -p "$file" $SYSCTL_ARGS || return 1
>> +	}
> 
> So it will only read the first file it finds ?

The "||" operator means "do the RHS if the LHS is false", so if "$file"
is empty it doesn't run sysctl; otherwise if syscl fails (which currently
never happens due to a bug in Busybox) it returns from the function.

-- 
Carlos Santos (Casantos) - DATACOM, P&D
?Marched towards the enemy, spear upright, armed with the certainty
that only the ignorant can have.? ? Epitaph of a volunteer

[Buildroot] [PATCH 1/2] package/procps-ng: add init script for sysctl

[Carlos Santos]

> From: "Arnout Vandecappelle" <arnout@mind.be>
> To: "Carlos Santos" <casantos@datacom.com.br>, "buildroot" <buildroot@buildroot.org>
> Cc: "ratbert90" <aduskett@gmail.com>, "DATACOM" <rodrigo.rebello@datacom.com.br>
> Sent: Ter?a-feira, 18 de dezembro de 2018 14:16:23
> Subject: Re: [Buildroot] [PATCH 1/2] package/procps-ng: add init script for sysctl

> Hi Carlos,
> 
> Since you're the init script master :-) let's do some bikeshedding!
> 
> On 18/12/2018 06:02, Carlos Santos wrote:
>> Add a simple init script that invokes sysctl early in the initialization
>> process to configure kernel parameters. This is already performed by
>> systemd (systemd-sysctl) but there is no sysvinit/busybox counterpart.
>> 
>> Files are read from directories in the following list in the given order
>> from top to bottom:
>> 
>>     /run/sysctl.d/*.conf
>>     /etc/sysctl.d/*.conf
>>     /usr/local/lib/sysctl.d/*.conf
>>     /usr/lib/sysctl.d/*.conf
>>     /lib/sysctl.d/*.conf
>>     /etc/sysctl.conf
>> 
>> Signed-off-by: Carlos Santos <casantos@datacom.com.br>
>> ---
>>  package/procps-ng/S01sysctl    | 41 ++++++++++++++++++++++++++++++++++
>>  package/procps-ng/procps-ng.mk |  5 +++++
>>  2 files changed, 46 insertions(+)
>>  create mode 100644 package/procps-ng/S01sysctl
>> 
>> diff --git a/package/procps-ng/S01sysctl b/package/procps-ng/S01sysctl
>> new file mode 100644
>> index 0000000000..e93bdb9e17
>> --- /dev/null
>> +++ b/package/procps-ng/S01sysctl
> 
> Is 01 the most appropriate place? I would tend to think that we want to log any
> errors from sysctl, so it should be 02 or 03.

Unfortunately sysctl does not use syslog. We could redirect its
std{out|err} to logger but no other init script does this and I
chose to keep it simple in this first version. I can make it a
bit more fancy, however.

>> @@ -0,0 +1,41 @@
>> +#!/bin/sh
>> +
>> +PROGRAM="sysctl"
> 
> In other init scripts you used DAEMON. Of course, in this case, it's not a
> daemon, but maybe for consistency it's better to keep the same?

I chose PROGRAM exactly because it is not a daemon. DAEMONs must be
started with start-stop-daemon and must have PID files.

>> +
>> +SYSCTL_ARGS=""
>> +
>> +# shellcheck source=/dev/null
>> +[ -r "/etc/default/$PROGRAM" ] && . "/etc/default/$PROGRAM"
>> +
>> +start() {
>> +	printf 'Starting %s: ' "$PROGRAM"
>> +	# shellcheck disable=SC2086 # we need the word splitting
>> +	/sbin/sysctl --quiet --system $SYSCTL_ARGS
> 
> I'm not so sure about the --quiet here. Even more, I would redirect the output
> into logger.

See above.

>> +	status=$?
>> +	if [ "$status" -eq 0 ]; then
>> +		echo "OK"
>> +	else
>> +		echo "FAIL"
>> +	fi
>> +	return "$status"
>> +}
>> +
>> +stop() {
>> +	printf 'Stopping %s: OK\n' "$PROGRAM"
> 
> Since nothing is actually done, I think it's better to not print anything.

A silent "stop" may lead the user to believe that something went
wrong. That's debatable, of course. I'm still torn between the two
options but for the moment I'd prefer to keep the message. It also
helps us to implement some automated test in the future.

>> +}
>> +
>> +restart() {
>> +	stop
>> +	sleep 1
> 
> And here I'd also skip the stop and more importantly the sleep.

Yeah, the sleep is useless.

>> +	start
>> +}
>> +
>> +case "$1" in
>> +        start|stop)
> 
> You put spaces here.
> 
>> +		"$1";;
>> +	restart|reload)
>> +		restart;;
> 
> So here, I'd directly go for 'start' instead of restart. So maybe
> 
> case "$1" in
>	start|restart|reload)
>		start;;
>	stop)
>		:;;
> 
> I'm not sure, what do you think? Is it better to keep the pattern for the case
> statement always the same (except for the restart|reload part)?
> 
> If the pattern should stay the same, in the other scripts you actually put:
> 
>        start|stop|restart)
>                "$1";;
>        reload)
>                # Restart, since there is no true "reload" feature.
>                restart;;

In this particular case I'd choose the first option.

> 
> 
> Regards,
> Arnout
> 
>> +        *)
>> +                echo "Usage: $0 {start|stop|restart|reload}"
>> +                exit 1
>> +esac
>> diff --git a/package/procps-ng/procps-ng.mk b/package/procps-ng/procps-ng.mk
>> index 03b74784d2..e40aeb5a2a 100644
>> --- a/package/procps-ng/procps-ng.mk
>> +++ b/package/procps-ng/procps-ng.mk
>> @@ -44,4 +44,9 @@ ifeq ($(BR2_STATIC_LIBS),y)
>>  PROCPS_NG_CONF_OPTS += --disable-numa
>>  endif
>>  
>> +define PROCPS_NG_INSTALL_INIT_SYSV
>> +	$(INSTALL) -D -m 755 package/procps-ng/S01sysctl \
>> +		$(TARGET_DIR)/etc/init.d/S01sysctl
>> +endef
>> +
>>  $(eval $(autotools-package))

-- 
Carlos Santos (Casantos) - DATACOM, P&D

[Buildroot] [PATCH 2/2] package/busybox: add init script for sysctl

[Carlos Santos]

> From: "Carlos Santos" <casantos@datacom.com.br>
> To: "Nicolas Cavallari" <Nicolas.Cavallari@green-communications.fr>
> Cc: "DATACOM" <rodrigo.rebello@datacom.com.br>, "ratbert90" <aduskett@gmail.com>, "buildroot" <buildroot@buildroot.org>
> Sent: Ter?a-feira, 18 de dezembro de 2018 15:19:49
> Subject: Re: [Buildroot] [PATCH 2/2] package/busybox: add init script for sysctl

>> From: "Nicolas Cavallari" <Nicolas.Cavallari@green-communications.fr>
>> To: "Carlos Santos" <casantos@datacom.com.br>, "buildroot"
>> <buildroot@buildroot.org>
>> Cc: "ratbert90" <aduskett@gmail.com>, "DATACOM" <rodrigo.rebello@datacom.com.br>
>> Sent: Ter?a-feira, 18 de dezembro de 2018 14:32:18
>> Subject: Re: [Buildroot] [PATCH 2/2] package/busybox: add init script for sysctl
> 
>> On 18/12/2018 06:02, Carlos Santos wrote:
>>> +	find $SYSCTL_SOURCES -maxdepth 1 -name '*.conf' -print0 2> /dev/null | \
>>> +	xargs -0 -r readlink -f | \
>>> +	{
>>> +		read -r file
>>> +		[ -z "$file" ] || /sbin/sysctl -q -p "$file" $SYSCTL_ARGS || return 1
>>> +	}
>> 
>> So it will only read the first file it finds ?
> 
> The "||" operator means "do the RHS if the LHS is false", so if "$file"
> is empty it doesn't run sysctl; otherwise if syscl fails (which currently
> never happens due to a bug in Busybox) it returns from the function.

Dooh, you were right. There should be a while there, so it is reading
only the first file name. I will fix this and send a new patch.

Thanks

-- 
Carlos Santos (Casantos) - DATACOM, P&D