[Buildroot] [PATCH 0/8] init scripts: rewrite S01logging

[Buildroot] [PATCH 0/8] init scripts: rewrite S01logging

[Carlos Santos]

Continuing our effort to improve daemon startup scripts. This series
focuses on S01logging, which starts the logging daemon. Common features
are:

- Indent with tabs, not spaces.
- Implement start, stop, restart and reload as functions.
- Use start-stop-daemon.
- Use logic operators && and || to detect/handle errors, which provides
  better readability than nested if/then/else blocks.
- Use brackets for blocking, also improving readability.
- Correctly Detect and report start/stop/restart/reload errors.
- Use separate functions for restart and reload; report the result of
  the whole operations instead of invoking stop, start and report OK
  twice.
- Support a configuration file at /etc/default (example files for each
  package will be added in separate patches).
- Support a configuration variable that completely disables the service
  and issues a warning message on any invocation.

The configuration files are provided mostly as examples for init script
authors but they also contain information about options that cannot be
used when running in background. 

All files implement the following FSM:

                                                  +---------+
             +-------stop--------+   +----(1s)----| stopped |
             |                   |   |            |   (*)   |
             |                   |   |            +---------+
             v                   |   v                 ^
      +---------+             +---------+              |
      |         |             |         |----restart---+
      | STOPPED |----start--->| STARTED |
      |         |             |         |----reload----+
      +---------+             +---------+              |
                                     ^                 |
                                     |                 |
                                     +-----------------+

* "stopped" is an intermediary state that transitions automatically to
  STARTED after one second.

Attempts to do invalid transitions (e.g. start from STARTED state) will
fail. That's why we don't pass -o (--oknodo) to start-stop-daemon. This
changes the script behavior, in some cases, while in other cases just
reports errors that were ignored previously.

We could interpret "start from STARTED" as "do nothing, ok" to match
systemctl (systemd). Leave such change for later, however, since the
current behavior matches most existing scripts.

Carlos Santos (8):
  busybox: update S01logging
  busybox: add logging configuration file
  rsyslog: update S01logging
  rsyslog: add logging configuration file
  sysklogd: update S01logging
  sysklogd: add logging configuration file
  rsyslog: update S01logging
  syslog-ng: add logging configuration file

 package/busybox/S01logging            | 87 +++++++++++++++++--------
 package/busybox/busybox.mk            |  2 +
 package/busybox/etc.default.logging   | 13 ++++
 package/rsyslog/S01logging            | 74 ++++++++++++++-------
 package/rsyslog/etc.default.logging   | 14 ++++
 package/rsyslog/rsyslog.mk            |  2 +
 package/sysklogd/S01logging           | 93 +++++++++++++++++++++------
 package/sysklogd/etc.default.logging  | 25 +++++++
 package/sysklogd/sysklogd.mk          |  2 +
 package/syslog-ng/S01logging          | 83 +++++++++++++++++-------
 package/syslog-ng/etc.default.logging | 14 ++++
 package/syslog-ng/syslog-ng.mk        |  2 +
 12 files changed, 316 insertions(+), 95 deletions(-)
 create mode 100644 package/busybox/etc.default.logging
 create mode 100644 package/rsyslog/etc.default.logging
 create mode 100644 package/sysklogd/etc.default.logging
 create mode 100644 package/syslog-ng/etc.default.logging

-- 
2.17.1

[Buildroot] [PATCH 1/8] busybox: update S01logging

[Carlos Santos]

Reformat and fix syslogd/klogd startup script for better quality and
code style:

- Indent with tabs, not spaces.
- Use logic operators && and || to detect/handle errors, which provides
  better readability than nested if/then/else blocks.
- Use brackets for blocking, also improving readability.
- Detect and report start/stop errors (previous version ignored them and
  always reported OK).
- Use a separate function for restart and report the result of the whole
  operation instead of invoking stop, start and report OK twice.
- Implement reload as restart, but with a nickname to report the result
  of the expected operation.
- Support a configuration variable that completely disables the service
  and issues a warning message on any invocation.

The script still has a bug: if klogd fails to load it reports FAIL but
leaves syslogd running. This is partially attenuated because a "stop"
would kill syslogd, fail on klogd and report "FAIL", but still do its
job. We could fix this by means of more detailed logic or splitting the
script but lets leave as is, since it is not a regression compared to
the previous situation.

Signed-off-by: Carlos Santos <casantos@datacom.com.br>
---
 package/busybox/S01logging | 87 ++++++++++++++++++++++++++------------
 1 file changed, 59 insertions(+), 28 deletions(-)

diff --git a/package/busybox/S01logging b/package/busybox/S01logging
index fcb3e7d236..12f20933cc 100644
--- a/package/busybox/S01logging
+++ b/package/busybox/S01logging
@@ -1,40 +1,71 @@
 #!/bin/sh
-#
-# Start logging
-#
 
-SYSLOGD_ARGS=-n
-KLOGD_ARGS=-n
+SYSLOGD_ARGS=""
+KLOGD_ARGS=""
+ENABLED="yes"
+
+# shellcheck source=/dev/null
 [ -r /etc/default/logging ] && . /etc/default/logging
 
+DAEMON="logging"
+
+test "$ENABLED" = "yes" || {
+	printf '%s is disabled\n' "$DAEMON"
+	exit 0
+}
+
+# BusyBox' syslogd and klogd don't create pidfiles, so use "-m" to instruct
+# start-stop-daemon to create them.
 start() {
-	printf "Starting logging: "
-	start-stop-daemon -b -S -q -m -p /var/run/syslogd.pid --exec /sbin/syslogd -- $SYSLOGD_ARGS
-	start-stop-daemon -b -S -q -m -p /var/run/klogd.pid --exec /sbin/klogd -- $KLOGD_ARGS
-	echo "OK"
+	printf 'Starting %s: ' "$DAEMON"
+	{
+		# shellcheck disable=SC2086 # we need the word splitting
+		start-stop-daemon -b -S -q -m -p /var/run/syslogd.pid -x /sbin/syslogd -- -n $SYSLOGD_ARGS && \
+		start-stop-daemon -b -S -q -m -p /var/run/klogd.pid -x /sbin/klogd -- -n $KLOGD_ARGS && \
+		echo "OK"
+	} || {
+		echo "FAIL"
+		exit 1
+	}
 }
 
 stop() {
-	printf "Stopping logging: "
-	start-stop-daemon -K -q -p /var/run/syslogd.pid
-	start-stop-daemon -K -q -p /var/run/klogd.pid
-	echo "OK"
+	printf 'Stopping %s: ' "$DAEMON"
+	{
+		start-stop-daemon -K -q -p /var/run/syslogd.pid && \
+		start-stop-daemon -K -q -p /var/run/klogd.pid && \
+		echo "OK"
+	} || {
+		echo "FAIL"
+		exit 1
+	}
+}
+
+restart() {
+	printf '%s %s: ' "${1:-Restarting}" "$DAEMON"
+	{
+		# shellcheck disable=SC2086 # we need the word splitting
+		start-stop-daemon -K -q -p /var/run/syslogd.pid && \
+		start-stop-daemon -K -q -p /var/run/klogd.pid && \
+		sleep 1 && \
+		start-stop-daemon -b -S -q -m -p /var/run/syslogd.pid -x /sbin/syslogd -- -n $SYSLOGD_ARGS && \
+		start-stop-daemon -b -S -q -m -p /var/run/klogd.pid -x /sbin/klogd -- -n $KLOGD_ARGS && \
+		echo "OK"
+	} || {
+		echo "FAIL"
+		exit 1
+	}
+}
+
+# BusyBox' syslogd and klogd ignore SIGHUP, SIGUSRx, so simply restart.
+reload() {
+	restart "Reloading"
 }
 
 case "$1" in
-  start)
-	start
-	;;
-  stop)
-	stop
-	;;
-  restart|reload)
-	stop
-	start
-	;;
-  *)
-	echo "Usage: $0 {start|stop|restart|reload}"
-	exit 1
+        start|stop|restart|reload)
+                "$1";;
+        *)
+                echo "Usage: $0 {start|stop|restart|reload}"
+                exit 1
 esac
-
-exit $?
-- 
2.17.1

[Buildroot] [PATCH 2/8] busybox: add logging configuration file

[Carlos Santos]

Provide a template to help users to customize syslogd and klogd without
editting the startup script.

syslogd options worth to configure are remote logging (-R), rotation
(-s, -b) and minimal priority level (-l).

klogd minimal priority level (-c) can be configured too, preventing
non-critical kernel messages from appearing on the console.

This file is also useful as an example for init script authors.

Signed-off-by: Carlos Santos <casantos@datacom.com.br>
---
 package/busybox/busybox.mk          |  2 ++
 package/busybox/etc.default.logging | 13 +++++++++++++
 2 files changed, 15 insertions(+)
 create mode 100644 package/busybox/etc.default.logging

diff --git a/package/busybox/busybox.mk b/package/busybox/busybox.mk
index 5266f844b6..fac2c394cf 100644
--- a/package/busybox/busybox.mk
+++ b/package/busybox/busybox.mk
@@ -252,6 +252,8 @@ define BUSYBOX_INSTALL_LOGGING_SCRIPT
 	then \
 		$(INSTALL) -m 0755 -D package/busybox/S01logging \
 			$(TARGET_DIR)/etc/init.d/S01logging; \
+		$(INSTALL) -m 0644 -D package/busybox/etc.default.logging \
+			$(TARGET_DIR)/etc/default/logging; \
 	fi
 endef
 
diff --git a/package/busybox/etc.default.logging b/package/busybox/etc.default.logging
new file mode 100644
index 0000000000..6775622779
--- /dev/null
+++ b/package/busybox/etc.default.logging
@@ -0,0 +1,13 @@
+#
+# /etc/default/logging (busybox version)
+#
+
+# Use SYSLOGD_ARGS to pass additional arguments to syslogd (e.g. for log
+# rotation).
+# SYSLOGD_ARGS="" # (default value)
+
+# Use KLOGD_ARGS to pass additional arguments to klogd.
+# KLOGD_ARGS="" # (default value)
+
+# Uncomment the line below to disable this service
+# ENABLED="no"
-- 
2.17.1

[Buildroot] [PATCH 3/8] rsyslog: update S01logging

[Carlos Santos]

Reformat and fix rsyslog startup script for better quality and code
style:

- Indent with tabs, not spaces.
- Use logic operators && and || to detect/handle errors, which provides
  better readability than nested if/then/else blocks.
- Use brackets for blocking, also improving readability.
- Rewrite restart to report the result of the whole operation instead of
  invoking stop, start and report OK twice.
- Add a "reload" option, implemented as restart, but with a nickname to
  report the result of the expected operation.
- Support a configuration file at /etc/default (an example file will be
  added in forthcomming patch).
- Support a configuration variable that completely disables the service
  and issues a warning message on any invocation.

Signed-off-by: Carlos Santos <casantos@datacom.com.br>
---
 package/rsyslog/S01logging | 74 ++++++++++++++++++++++++++------------
 1 file changed, 51 insertions(+), 23 deletions(-)

diff --git a/package/rsyslog/S01logging b/package/rsyslog/S01logging
index 8e4a59c2d5..94b5a1bd96 100644
--- a/package/rsyslog/S01logging
+++ b/package/rsyslog/S01logging
@@ -1,36 +1,64 @@
 #!/bin/sh
 
+RSYSLOGD_ARGS=""
+ENABLED="yes"
+
+# shellcheck source=/dev/null
+[ -r /etc/default/logging ] && . /etc/default/logging
+
+DAEMON="rsyslogd"
+
+test "$ENABLED" = "yes" || {
+	printf '%s is disabled\n' "$DAEMON"
+	exit 0
+}
+
 start() {
-  printf "Starting rsyslog daemon: "
-  start-stop-daemon -S -q -p /var/run/rsyslogd.pid --exec /usr/sbin/rsyslogd
-  [ $? = 0 ] && echo "OK" || echo "FAIL"
+	printf 'Starting %s: ' "$DAEMON"
+	{
+		# shellcheck disable=SC2086 # we need the word splitting
+		start-stop-daemon -b -S -q -p /var/run/rsyslogd.pid -x /usr/sbin/rsyslogd -- -n $RSYSLOGD_ARGS && \
+		echo "OK"
+	} || {
+		echo "FAIL"
+		exit 1
+	}
 }
 
 stop() {
-  printf "Stopping rsyslog daemon: "
-  start-stop-daemon -K -q -p /var/run/rsyslogd.pid
-  [ $? = 0 ] && echo "OK" || echo "FAIL"
+	printf 'Stopping %s: ' "$DAEMON"
+	{
+		start-stop-daemon -K -q -p /var/run/rsyslogd.pid && \
+		echo "OK"
+	} || {
+		echo "FAIL"
+		exit 1
+	}
 }
 
 restart() {
-  stop
-  sleep 1
-  start
+	printf '%s %s: ' "${1:-Restarting}" "$DAEMON"
+	{
+		# shellcheck disable=SC2086 # we need the word splitting
+		start-stop-daemon -K -q -p /var/run/rsyslogd.pid && \
+		sleep 1 && \
+		start-stop-daemon -b -S -q -p /var/run/rsyslogd.pid -x /usr/sbin/rsyslogd -- -n $RSYSLOGD_ARGS && \
+		echo "OK"
+	} || {
+		echo "FAIL"
+		exit 1
+	}
+}
+
+# rsyslogd ignores SIGHUP, SIGUSRx, so simply restart.
+reload() {
+	restart "Reloading"
 }
 
 case "$1" in
-  start)
-    start
-    ;;
-  stop)
-    stop
-    ;;
-  restart|reload)
-    restart
-    ;;
-  *)
-    echo "Usage: $0 {start|stop|restart}"
-    exit 1
+        start|stop|restart|reload)
+                "$1";;
+        *)
+                echo "Usage: $0 {start|stop|restart|reload}"
+                exit 1
 esac
-
-exit $?
-- 
2.17.1

[Buildroot] [PATCH 4/8] rsyslog: add logging configuration file

[Carlos Santos]

Provide a template to help users to customize rsyslog without editting
the startup script. Also warn about options that must not be used.

Add instructions on how to configure debugging, since it a bit tricky
when rsyslogd runs in background, requiring a separete log file.

This file is also useful as an example for init script authors.

Signed-off-by: Carlos Santos <casantos@datacom.com.br>
---
 package/rsyslog/etc.default.logging | 14 ++++++++++++++
 package/rsyslog/rsyslog.mk          |  2 ++
 2 files changed, 16 insertions(+)
 create mode 100644 package/rsyslog/etc.default.logging

diff --git a/package/rsyslog/etc.default.logging b/package/rsyslog/etc.default.logging
new file mode 100644
index 0000000000..41b7a9d779
--- /dev/null
+++ b/package/rsyslog/etc.default.logging
@@ -0,0 +1,14 @@
+#
+# /etc/default/logging (rsyslog version)
+#
+
+# Use RSYSLOGD_ARGS to pass additional arguments to rsyslogd.
+# - Do NOT use "-i pid_file" nor "-v", since they will break the startup script.
+# - Use "-d" to enable debug. Warning: may become HUGE!
+# RSYSLOGD_ARGS="" # (default value)
+
+# You will need this along with "-d".
+# RSYSLOG_DEBUGLOG="/var/log/rsyslogd"; export RSYSLOG_DEBUGLOG
+
+# Uncomment the line below to disable this service
+# ENABLED="no"
diff --git a/package/rsyslog/rsyslog.mk b/package/rsyslog/rsyslog.mk
index 61e08ba765..c80d0696e2 100644
--- a/package/rsyslog/rsyslog.mk
+++ b/package/rsyslog/rsyslog.mk
@@ -74,6 +74,8 @@ endif
 define RSYSLOG_INSTALL_INIT_SYSV
 	$(INSTALL) -m 0755 -D package/rsyslog/S01logging \
 		$(TARGET_DIR)/etc/init.d/S01logging
+	$(INSTALL) -m 0644 -D package/rsyslog/etc.default.logging \
+		$(TARGET_DIR)/etc/default/logging
 endef
 
 # The rsyslog.service is installed by rsyslog, but the link is not created
-- 
2.17.1

[Buildroot] [PATCH 5/8] sysklogd: update S01logging

[Carlos Santos]

Reformat and fix syslogd/klogd startup script for better quality and
code style:

- Implement start, stop, restart and reload as functions, like in other
  S01logging scripts, using start-stop-daemon.
- Indent with tabs, not spaces.
- Use logic operators && and || to detect/handle errors, which provides
  better readability than nested if/then/else blocks.
- Use brackets for blocking, also improving readability.
- Detect and report start/stop errors (previous version ignored them and
  always reported OK).
- Use a separate function for restart and report the result of the whole
  operation instead of invoking stop, start and report OK twice.
- Support a configuration file at /etc/default (an example file will be
  added in forthcomming patch).
- Support a configuration variable that completely disables the service
  and issues a warning message on any invocation.
- Do not kill syslogd in "reload". Send a SIGHUP signal, instructing it
  to perform a re-initialization. Also do not kill klogd. Send a signal
  (default 0, which does nothing). Users can configure this signal in
  /etc/default/logging to either SIGUSR1 or SIGUSR2.

The script still has a bug: if klogd fails to load it reports FAIL but
leaves syslogd running. This is partially attenuated because a "stop"
would kill syslogd, fail on klogd and report "FAIL", but still do its
job. We could fix this by means of more detailed logic or splitting the
script but lets leave as is, since it is not a regression compared to
the previous situation.

Signed-off-by: Carlos Santos <casantos@datacom.com.br>
---
 package/sysklogd/S01logging | 93 +++++++++++++++++++++++++++++--------
 1 file changed, 74 insertions(+), 19 deletions(-)

diff --git a/package/sysklogd/S01logging b/package/sysklogd/S01logging
index 1cbfe869fa..16e45906dc 100644
--- a/package/sysklogd/S01logging
+++ b/package/sysklogd/S01logging
@@ -1,25 +1,80 @@
 #!/bin/sh
 
-case "$1" in
-	start)
-		printf "Starting logging: "
-		/sbin/syslogd -m 0
-		/sbin/klogd
+SYSLOGD_ARGS="-m 0"
+KLOGD_ARGS=""
+KLOGD_RELOAD="0"
+ENABLED="yes"
+
+# shellcheck source=/dev/null
+[ -r /etc/default/logging ] && . /etc/default/logging
+
+DAEMON="sysklogd"
+
+test "$ENABLED" = "yes" || {
+	printf '%s is disabled\n' "$DAEMON"
+	exit 0
+}
+
+start() {
+	printf 'Starting %s: ' "$DAEMON"
+	{
+		# shellcheck disable=SC2086 # we need the word splitting
+		start-stop-daemon -b -S -q -p /var/run/syslogd.pid -x /sbin/syslogd -- -n $SYSLOGD_ARGS && \
+		start-stop-daemon -b -S -q -p /var/run/klogd.pid -x /sbin/klogd -- -n $KLOGD_ARGS && \
 		echo "OK"
-		;;
-	stop)
-		printf "Stopping logging: "
-		[ -f /var/run/klogd.pid ] && kill `cat /var/run/klogd.pid`
-		[ -f /var/run/syslogd.pid ] && kill `cat /var/run/syslogd.pid`
+	} || {
+		echo "FAIL"
+		exit 1
+	}
+}
+
+stop() {
+	printf 'Stopping %s: ' "$DAEMON"
+	{
+		start-stop-daemon -K -q -p /var/run/syslogd.pid && \
+		start-stop-daemon -K -q -p /var/run/klogd.pid && \
 		echo "OK"
-		;;
-	restart|reload)
-		$0 stop
-		$0 start
-		;;
-	*)
-		echo "Usage: $0 {start|stop|restart}"
+	} || {
+		echo "FAIL"
 		exit 1
-esac
+	}
+}
 
-exit $?
+restart() {
+	printf 'Restartng %s: ' "$DAEMON"
+	{
+		# shellcheck disable=SC2086 # we need the word splitting
+		start-stop-daemon -K -q -p /var/run/syslogd.pid && \
+		start-stop-daemon -K -q -p /var/run/klogd.pid && \
+		sleep 1 && \
+		start-stop-daemon -b -S -q -p /var/run/syslogd.pid -x /sbin/syslogd -- -n $SYSLOGD_ARGS && \
+		start-stop-daemon -b -S -q -p /var/run/klogd.pid -x /sbin/klogd -- -n $KLOGD_ARGS && \
+		echo "OK"
+	} || {
+		echo "FAIL"
+		exit 1
+	}
+}
+
+# SIGHUP makes syslogd reload its configuration
+# SIGUSR1 makes klogd reload kernel module symbols
+# SIGUSR2 makes klogd reload static kernel symbols and kernel module symbols
+reload() {
+	printf 'Reloading %s: ' "$DAEMON"
+	{
+		start-stop-daemon -K -s HUP -q -p /var/run/syslogd.pid && \
+		start-stop-daemon -K -s $KLOGD_RELOAD -q -p /var/run/klogd.pid && \
+		echo "OK"
+	} || {
+		echo "FAIL"
+		exit 1
+	}
+}
+
+case "$1" in
+        start|stop|restart|reload)
+                "$1";;
+        *)
+                echo "Usage: $0 {start|stop|restart|reload}"
+                exit 1
+esac
-- 
2.17.1

[Buildroot] [PATCH 6/8] sysklogd: add logging configuration file

[Carlos Santos]

Provide a template to help users to customize syslogd and klogd without
editting the startup script. Also warn about options that must not be
used.

syslogd options worth to configure are mark interval (-m), remote log
support (-r) and domain list (-s).

Give example of how to configure klogd re-initialization signal.

Signed-off-by: Carlos Santos <casantos@datacom.com.br>
---
 package/sysklogd/etc.default.logging | 25 +++++++++++++++++++++++++
 package/sysklogd/sysklogd.mk         |  2 ++
 2 files changed, 27 insertions(+)
 create mode 100644 package/sysklogd/etc.default.logging

diff --git a/package/sysklogd/etc.default.logging b/package/sysklogd/etc.default.logging
new file mode 100644
index 0000000000..d6e4410028
--- /dev/null
+++ b/package/sysklogd/etc.default.logging
@@ -0,0 +1,25 @@
+#
+# /etc/default/logging (sysklog version)
+#
+
+# Use SYSLOGD_ARGS to pass additional arguments to syslogd.
+# - Do NOT use "-v", since it will break the startup script.
+# - Do NOT use "-d", since debugging does not work when running in background.
+# SYSLOGD_ARGS="-m 0" # (default value)
+
+# Use KLOGD_ARGS to pass additional arguments to klogd.
+# - Do NOT use "-o" nor "-v", since they will break the startup script.
+# - Do NOT use "-d", since debugging does not work when running in background.
+# KLOGD_ARGS="-m 0" # (default value)
+
+# Use KLOGD_RELOAD to select the "S01logging reload" behavior.
+# - "USR1" will cause the kernel module symbols to be reloaded.
+# - "USR2" will cause both the static kernel symbols and the kernel module
+#   symbols to be reloaded.
+# - "0" will do nothing.
+# - Do not use any other value here. Use kill(1) for fine-grained control of
+#   klogd, as documented in its manual page.
+# KLOGD_RELOAD="0" # (default value)
+
+# Uncomment the line below to disable this service
+# ENABLED="no"
diff --git a/package/sysklogd/sysklogd.mk b/package/sysklogd/sysklogd.mk
index c4f064c10b..b7e67ffee3 100644
--- a/package/sysklogd/sysklogd.mk
+++ b/package/sysklogd/sysklogd.mk
@@ -25,6 +25,8 @@ endef
 define SYSKLOGD_INSTALL_INIT_SYSV
 	$(INSTALL) -m 755 -D package/sysklogd/S01logging \
 		$(TARGET_DIR)/etc/init.d/S01logging
+	$(INSTALL) -m 0644 -D package/sysklogd/etc.default.logging \
+		$(TARGET_DIR)/etc/default/logging
 endef
 
 define SYSKLOGD_INSTALL_INIT_SYSTEMD
-- 
2.17.1

[Buildroot] [PATCH 7/8] rsyslog: update S01logging

[Carlos Santos]

Reformat and fix syslog-ng startup script for better quality and code
style:

- Indent with tabs, not spaces.
- Use logic operators && and || to detect/handle errors, which provides
  better readability than nested if/then/else blocks.
- Use brackets for blocking, also improving readability.
- Rewrite restart to report the result of the whole operation instead of
  invoking stop, start and report OK twice.
- Do not kill syslog-ng in "reload". Send a SIGHUP signal, instructing
  it to perform a re-initialization.
- Support a configuration file at /etc/default (an example file will be
  added in forthcomming patch).
- Support a configuration variable that completely disables the service
  and issues a warning message on any invocation.

Signed-off-by: Carlos Santos <casantos@datacom.com.br>
---
 package/syslog-ng/S01logging | 83 +++++++++++++++++++++++++-----------
 1 file changed, 58 insertions(+), 25 deletions(-)

diff --git a/package/syslog-ng/S01logging b/package/syslog-ng/S01logging
index d7c899a1e3..ba2eb226c1 100644
--- a/package/syslog-ng/S01logging
+++ b/package/syslog-ng/S01logging
@@ -1,38 +1,71 @@
 #!/bin/sh
 
+SYSLOG_NG_ARGS=""
+ENABLED="yes"
+
+# shellcheck source=/dev/null
+[ -r /etc/default/logging ] && . /etc/default/logging
+
+DAEMON="syslog-ng"
+
+test "$ENABLED" = "yes" || {
+	printf '%s is disabled\n' "$DAEMON"
+	exit 0
+}
+
 start() {
-	printf "Starting syslog-ng daemon: "
-	start-stop-daemon -S -q -p /var/run/syslog-ng.pid \
-		-x /usr/sbin/syslog-ng -- --pidfile /var/run/syslog-ng.pid
-	[ $? = 0 ] && echo "OK" || echo "FAIL"
+	printf 'Starting %s: ' "$DAEMON"
+	{
+		# shellcheck disable=SC2086 # we need the word splitting
+		start-stop-daemon -b -S -q -p /var/run/syslog-ng.pid -x /usr/sbin/syslog-ng -- -F $SYSLOG_NG_ARGS && \
+		echo "OK"
+	} || {
+		echo "FAIL"
+		exit 1
+	}
 }
 
 stop() {
-	printf "Stopping syslog-ng daemon: "
-	start-stop-daemon -K -q -p /var/run/syslog-ng.pid \
-		-x /usr/sbin/syslog-ng
-	[ $? = 0 ] && echo "OK" || echo "FAIL"
+	printf 'Stopping %s: ' "$DAEMON"
+	{
+		start-stop-daemon -K -q -p /var/run/syslog-ng.pid && \
+		echo "OK"
+	} || {
+		echo "FAIL"
+		exit 1
+	}
 }
 
 restart() {
-	stop
-	sleep 1
-	start
+	printf 'Restartng %s: ' "$DAEMON"
+	{
+		# shellcheck disable=SC2086 # we need the word splitting
+		start-stop-daemon -K -q -p /var/run/syslog-ng.pid && \
+		sleep 1 && \
+		start-stop-daemon -b -S -q -p /var/run/syslog-ng.pid -x /usr/sbin/syslog-ng -- -F $SYSLOG_NG_ARGS && \
+		echo "OK"
+	} || {
+		echo "FAIL"
+		exit 1
+	}
 }
 
-case "$1" in
-	start)
-		start
-		;;
-	stop)
-		stop
-		;;
-	restart|reload)
-		restart
-		;;
-	*)
-		echo "Usage: $0 {start|stop|restart}"
+# SIGHUP makes syslog-ng reload its configuration
+reload() {
+	printf 'Reloading %s: ' "$DAEMON"
+	{
+		start-stop-daemon -K -s HUP -q -p /var/run/syslog-ng.pid && \
+		echo "OK"
+	} || {
+		echo "FAIL"
 		exit 1
-esac
+	}
+}
 
-exit $?
+case "$1" in
+        start|stop|restart|reload)
+                "$1";;
+        *)
+                echo "Usage: $0 {start|stop|restart|reload}"
+                exit 1
+esac
-- 
2.17.1

[Buildroot] [PATCH 8/8] syslog-ng: add logging configuration file

[Carlos Santos]

Provide a template to help users to customize syslog-ng without editting
the startup script. Mostly warn about options that must not be used.

This file is also useful as an example for init script authors.

Signed-off-by: Carlos Santos <casantos@datacom.com.br>
---
 package/syslog-ng/etc.default.logging | 14 ++++++++++++++
 package/syslog-ng/syslog-ng.mk        |  2 ++
 2 files changed, 16 insertions(+)
 create mode 100644 package/syslog-ng/etc.default.logging

diff --git a/package/syslog-ng/etc.default.logging b/package/syslog-ng/etc.default.logging
new file mode 100644
index 0000000000..6160bfee9d
--- /dev/null
+++ b/package/syslog-ng/etc.default.logging
@@ -0,0 +1,14 @@
+#
+# /etc/default/logging (syslog-ng version)
+#
+
+# Use SYSLOG_NG_ARGS to pass additional arguments to rsyslogd.
+# - Do NOT use "--help"/"-h", "--version"/"-V", "--stderr"/"-e",
+#   "--syntax-only"/"-s", or "--process-mode=<background|safe-background>",
+#   since they will break the startup script.
+# - Do NOT use "--debug"/"-d", since debugging does not work when running in
+#   background.
+# SYSLOG_NG_ARGS="" # (default value)
+
+# Uncomment the line below to disable this service
+# ENABLED="no"
diff --git a/package/syslog-ng/syslog-ng.mk b/package/syslog-ng/syslog-ng.mk
index 793fea0972..a837dad841 100644
--- a/package/syslog-ng/syslog-ng.mk
+++ b/package/syslog-ng/syslog-ng.mk
@@ -95,6 +95,8 @@ endif
 define SYSLOG_NG_INSTALL_INIT_SYSV
 	$(INSTALL) -m 0755 -D package/syslog-ng/S01logging \
 		$(TARGET_DIR)/etc/init.d/S01logging
+	$(INSTALL) -m 0644 -D package/syslog-ng/etc.default.logging \
+		$(TARGET_DIR)/etc/default/logging
 endef
 
 # By default syslog-ng installs a number of sample configuration
-- 
2.17.1

[Buildroot] [PATCH 3/8] rsyslog: update S01logging

[Nicolas Cavallari]

Hello,

On 09/07/2018 05:31, Carlos Santos wrote:
> --- a/package/rsyslog/S01logging
> +++ b/package/rsyslog/S01logging
> @@ -1,36 +1,64 @@
>  #!/bin/sh
>  
> +RSYSLOGD_ARGS=""
> +ENABLED="yes"
> +
> +# shellcheck source=/dev/null
> +[ -r /etc/default/logging ] && . /etc/default/logging
> +
> +DAEMON="rsyslogd"
> +
> +test "$ENABLED" = "yes" || {

I find it strange to use test here and use [ ] everywhere else.
And why not use if here ?

> +	printf '%s is disabled\n' "$DAEMON"
> +	exit 0
> +}
> +
>  start() {
> -  printf "Starting rsyslog daemon: "
> -  start-stop-daemon -S -q -p /var/run/rsyslogd.pid --exec /usr/sbin/rsyslogd
> -  [ $? = 0 ] && echo "OK" || echo "FAIL"
> +	printf 'Starting %s: ' "$DAEMON"
> +	{
> +		# shellcheck disable=SC2086 # we need the word splitting
> +		start-stop-daemon -b -S -q -p /var/run/rsyslogd.pid -x /usr/sbin/rsyslogd -- -n $RSYSLOGD_ARGS && \

Why do you disable backgrounding in rsyslog and ask start-stop-daemon to do it
instead ?
This may prevent rsyslogd to report errors and exit 1 before it daemonize itself.

> +		echo "OK"
> +	} || {
> +		echo "FAIL"
> +		exit 1
> +	}
>  }


>  restart() {
> -  stop
> -  sleep 1
> -  start
> +	printf '%s %s: ' "${1:-Restarting}" "$DAEMON"
> +	{
> +		# shellcheck disable=SC2086 # we need the word splitting
> +		start-stop-daemon -K -q -p /var/run/rsyslogd.pid && \
> +		sleep 1 && \
> +		start-stop-daemon -b -S -q -p /var/run/rsyslogd.pid -x /usr/sbin/rsyslogd -- -n $RSYSLOGD_ARGS && \

This duplicates code, and the error reporting is less precise than before.  It
will just say "FAIL" if the daemon is not running or fails to start.

> +		echo "OK"
> +	} || {
> +		echo "FAIL"
> +		exit 1
> +	}
> +}
> +
> +# rsyslogd ignores SIGHUP, SIGUSRx, so simply restart.

Are you sure about that ? the man page says otherwise.

[Buildroot] [PATCH 1/8] busybox: update S01logging

[Nicolas Cavallari]

On 09/07/2018 05:31, Carlos Santos wrote:
> The script still has a bug: if klogd fails to load it reports FAIL but
> leaves syslogd running. This is partially attenuated because a "stop"
> would kill syslogd, fail on klogd and report "FAIL", but still do its
> job. We could fix this by means of more detailed logic or splitting the
> script but lets leave as is, since it is not a regression compared to
> the previous situation.

Considering the criticality of logging, maybe the old behavior should be kept:
the script should attempt to start both daemon regardless of if any of them fails.

This does not need to be complicated:

local err=0
start-stop-daemon --overlong-options || err=$?
start-stop-another-daemon --more-overlong-options || err=$?
if [ "$err" != 0 ]; then
    echo OK
else
    echo FAIL
    return $err
fi

> +restart() {
> +	printf '%s %s: ' "${1:-Restarting}" "$DAEMON"
> +	{
> +		# shellcheck disable=SC2086 # we need the word splitting
> +		start-stop-daemon -K -q -p /var/run/syslogd.pid && \
> +		start-stop-daemon -K -q -p /var/run/klogd.pid && \
> +		sleep 1 && \
> +		start-stop-daemon -b -S -q -m -p /var/run/syslogd.pid -x /sbin/syslogd -- -n $SYSLOGD_ARGS && \
> +		start-stop-daemon -b -S -q -m -p /var/run/klogd.pid -x /sbin/klogd -- -n $KLOGD_ARGS && \
> +		echo "OK"
> +	} || {
> +		echo "FAIL"
> +		exit 1
> +	}
> +}
> +
> +# BusyBox' syslogd and klogd ignore SIGHUP, SIGUSRx, so simply restart.
> +reload() {
> +	restart "Reloading"

I think we shouldn't hide the fact that we are restarting the daemon instead of
reloading it.

>  }

[Buildroot] [PATCH 0/8] init scripts: rewrite S01logging

[Arnout Vandecappelle]

 Hi Carlos,

 Thanks for doing this!

 I'm going to give feedback more on the principles than on the detailed
implementation. Most of this feedback is my personal opinion, so wait for input
from some other people before reworking it. If/when you repost, it would be a
good idea to summarize the feedback (including who said what, especially when
there are differences of opinion) in either the cover letter or below the --- line.

On 09-07-18 05:31, Carlos Santos wrote:
> Continuing our effort to improve daemon startup scripts. This series
> focuses on S01logging, which starts the logging daemon. Common features

 Good choice of something to focus on!

> are:
> 
> - Indent with tabs, not spaces.

 +1.

> - Implement start, stop, restart and reload as functions.

 +1.

> - Use start-stop-daemon.

 +1.

 I think these three are entirely uncontroversial, so if you'd make a separate
patch with just these three, it could already be applied.

> - Use logic operators && and || to detect/handle errors, which provides
>   better readability than nested if/then/else blocks.

 Not sure if I'd want to make a general rule out of that.

> - Use brackets for blocking, also improving readability.

 ... Proving my point: with if, you get blocking automatically.

> - Correctly Detect and report start/stop/restart/reload errors.

 +1. But that's closely related with the conditional structure so not worth
making a separate patch of it.

> - Use separate functions for restart and reload; report the result of
>   the whole operations instead of invoking stop, start and report OK
>   twice.

 Actually, I like to have the separate stop and start output.

 Remember that the restart and reload calls are only used for explicit calls,
not automatically by init. So a little extra verbosity here doesn't hurt at all IMO.

> - Support a configuration file at /etc/default (example files for each
>   package will be added in separate patches).

 I'm not convinced that example files are enough of an added value. If we always
use the same pattern in the init scripts, and we put default assignments of
these variables at the beginning of the init script, we can just explain in the
documentation that the variables that can be used in /etc/default/foo are
explained at the beginning of /etc/init.d/S??foo.

> - Support a configuration variable that completely disables the service
>   and issues a warning message on any invocation.

 Why would we want that? If you really want to disable it entirely, then just
remove the init script in a post-build script. Or make it non-executable if you
want to temporarily disable.


> The configuration files are provided mostly as examples for init script
> authors but they also contain information about options that cannot be
> used when running in background.

 That can be done at the beginning of the init script as well.


> All files implement the following FSM:
> 
>                                                   +---------+
>              +-------stop--------+   +----(1s)----| stopped |
>              |                   |   |            |   (*)   |
>              |                   |   |            +---------+
>              v                   |   v                 ^
>       +---------+             +---------+              |
>       |         |             |         |----restart---+
>       | STOPPED |----start--->| STARTED |
>       |         |             |         |----reload----+
>       +---------+             +---------+              |
>                                      ^                 |
>                                      |                 |
>                                      +-----------------+
> 
> * "stopped" is an intermediary state that transitions automatically to
>   STARTED after one second.

 I don't know if the 1s sleep makes sense in general. Although, indeed,
otherwise we'd have to check if the process is really dead before restarting.


> Attempts to do invalid transitions (e.g. start from STARTED state) will
> fail. That's why we don't pass -o (--oknodo) to start-stop-daemon. This
> changes the script behavior, in some cases, while in other cases just
> reports errors that were ignored previously.

 NACK that. You want to be able to do /etc/init.d/S??foo restart even if foo is
not actually running. So for start we indeed want to terminate, but for stop we
want to continue (and report failure).

> We could interpret "start from STARTED" as "do nothing, ok" to match
> systemctl (systemd). Leave such change for later, however, since the
> current behavior matches most existing scripts.

 +1


 Regards,
 Arnout

> 
> Carlos Santos (8):
>   busybox: update S01logging
>   busybox: add logging configuration file
>   rsyslog: update S01logging
>   rsyslog: add logging configuration file
>   sysklogd: update S01logging
>   sysklogd: add logging configuration file
>   rsyslog: update S01logging
>   syslog-ng: add logging configuration file
> 
>  package/busybox/S01logging            | 87 +++++++++++++++++--------
>  package/busybox/busybox.mk            |  2 +
>  package/busybox/etc.default.logging   | 13 ++++
>  package/rsyslog/S01logging            | 74 ++++++++++++++-------
>  package/rsyslog/etc.default.logging   | 14 ++++
>  package/rsyslog/rsyslog.mk            |  2 +
>  package/sysklogd/S01logging           | 93 +++++++++++++++++++++------
>  package/sysklogd/etc.default.logging  | 25 +++++++
>  package/sysklogd/sysklogd.mk          |  2 +
>  package/syslog-ng/S01logging          | 83 +++++++++++++++++-------
>  package/syslog-ng/etc.default.logging | 14 ++++
>  package/syslog-ng/syslog-ng.mk        |  2 +
>  12 files changed, 316 insertions(+), 95 deletions(-)
>  create mode 100644 package/busybox/etc.default.logging
>  create mode 100644 package/rsyslog/etc.default.logging
>  create mode 100644 package/sysklogd/etc.default.logging
>  create mode 100644 package/syslog-ng/etc.default.logging
> 

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [PATCH 1/8] busybox: update S01logging

[Arnout Vandecappelle]

On 09-07-18 10:33, Nicolas Cavallari wrote:
> On 09/07/2018 05:31, Carlos Santos wrote:
>> The script still has a bug: if klogd fails to load it reports FAIL but
>> leaves syslogd running. This is partially attenuated because a "stop"
>> would kill syslogd, fail on klogd and report "FAIL", but still do its
>> job. We could fix this by means of more detailed logic or splitting the
>> script but lets leave as is, since it is not a regression compared to
>> the previous situation.
> 
> Considering the criticality of logging, maybe the old behavior should be kept:
> the script should attempt to start both daemon regardless of if any of them fails.

 Well, klogd is anyway pretty useless without running syslogd. However...

> This does not need to be complicated:
> 
> local err=0
> start-stop-daemon --overlong-options || err=$?
> start-stop-another-daemon --more-overlong-options || err=$?
> if [ "$err" != 0 ]; then
>     echo OK
> else
>     echo FAIL
>     return $err
> fi

 This code is IMO a lot more readable than the code proposed by Carlos, so I
like it.

 Actually, I wouldn't mind a separate message for syslogd and klogd. But that's
another story again...

> 
>> +restart() {
>> +	printf '%s %s: ' "${1:-Restarting}" "$DAEMON"
>> +	{
>> +		# shellcheck disable=SC2086 # we need the word splitting
>> +		start-stop-daemon -K -q -p /var/run/syslogd.pid && \
>> +		start-stop-daemon -K -q -p /var/run/klogd.pid && \
>> +		sleep 1 && \
>> +		start-stop-daemon -b -S -q -m -p /var/run/syslogd.pid -x /sbin/syslogd -- -n $SYSLOGD_ARGS && \
>> +		start-stop-daemon -b -S -q -m -p /var/run/klogd.pid -x /sbin/klogd -- -n $KLOGD_ARGS && \
>> +		echo "OK"
>> +	} || {
>> +		echo "FAIL"
>> +		exit 1
>> +	}
>> +}
>> +
>> +# BusyBox' syslogd and klogd ignore SIGHUP, SIGUSRx, so simply restart.
>> +reload() {
>> +	restart "Reloading"
> 
> I think we shouldn't hide the fact that we are restarting the daemon instead of
> reloading it.

 For me, the old way of

restart|reload)
	stop
	sleep 1
	start
	;;

was perfectly fine. It avoids code duplication, and it does exactly what we need
to do. Well, to make it work, the exit 1 in case of failure would have to be
replaced with return 1.


 Regards,
 Arnout

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [PATCH 1/8] busybox: update S01logging

[Arnout Vandecappelle]

 More bikeshedding :-)

On 09-07-18 05:31, Carlos Santos wrote:

> +
> +# shellcheck source=/dev/null
>  [ -r /etc/default/logging ] && . /etc/default/logging
>  
> +DAEMON="logging"

 You could also use the DAEMON variable on the line above, to make it more
generic. So /etc/default/$DAEMON.

 For this particular one it is not possible, but for many daemons you can just
copy an existing init script and replace DAEMON with a different name.


> +# BusyBox' syslogd and klogd don't create pidfiles, so use "-m" to instruct
> +# start-stop-daemon to create them.

 Actually, the important thing is that with -m you need -b as well, which means
you *have* to pass -n on the syslogd/klogd command line.

>  start() {
> -	printf "Starting logging: "
> -	start-stop-daemon -b -S -q -m -p /var/run/syslogd.pid --exec /sbin/syslogd -- $SYSLOGD_ARGS
> -	start-stop-daemon -b -S -q -m -p /var/run/klogd.pid --exec /sbin/klogd -- $KLOGD_ARGS
> -	echo "OK"
> +	printf 'Starting %s: ' "$DAEMON"
> +	{
> +		# shellcheck disable=SC2086 # we need the word splitting
> +		start-stop-daemon -b -S -q -m -p /var/run/syslogd.pid -x /sbin/syslogd -- -n $SYSLOGD_ARGS && \
> +		start-stop-daemon -b -S -q -m -p /var/run/klogd.pid -x /sbin/klogd -- -n $KLOGD_ARGS && \
> +		echo "OK"
> +	} || {
> +		echo "FAIL"
> +		exit 1

 I think this should be "return 1". Since start is the last thing called from
the script, its return value will be used as the script exit value. For start it
doesn't matter much, but for stop it's an elegant way to make sure that restart
(by calling stop; start) does the start even if stop failed (because the daemon
anyway wasn't running yet).

> +	}
>  }
>  
>  stop() {
> -	printf "Stopping logging: "
> -	start-stop-daemon -K -q -p /var/run/syslogd.pid
> -	start-stop-daemon -K -q -p /var/run/klogd.pid
> -	echo "OK"
> +	printf 'Stopping %s: ' "$DAEMON"
> +	{
> +		start-stop-daemon -K -q -p /var/run/syslogd.pid && \
> +		start-stop-daemon -K -q -p /var/run/klogd.pid && \

 It would make more sense to revert the order. But yes, that's yet another
change compared to current behaviour.

 Regards,
 Arnout

[snip]
-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [PATCH 3/8] rsyslog: update S01logging

[Arnout Vandecappelle]

On 09-07-18 05:31, Carlos Santos wrote:
> Reformat and fix rsyslog startup script for better quality and code
> style:
> 
> - Indent with tabs, not spaces.
> - Use logic operators && and || to detect/handle errors, which provides
>   better readability than nested if/then/else blocks.
> - Use brackets for blocking, also improving readability.
> - Rewrite restart to report the result of the whole operation instead of
>   invoking stop, start and report OK twice.
> - Add a "reload" option, implemented as restart, but with a nickname to
>   report the result of the expected operation.
> - Support a configuration file at /etc/default (an example file will be
>   added in forthcomming patch).
> - Support a configuration variable that completely disables the service
>   and issues a warning message on any invocation.
> 
> Signed-off-by: Carlos Santos <casantos@datacom.com.br>
> ---
>  package/rsyslog/S01logging | 74 ++++++++++++++++++++++++++------------
>  1 file changed, 51 insertions(+), 23 deletions(-)
> 
> diff --git a/package/rsyslog/S01logging b/package/rsyslog/S01logging
> index 8e4a59c2d5..94b5a1bd96 100644
> --- a/package/rsyslog/S01logging
> +++ b/package/rsyslog/S01logging
> @@ -1,36 +1,64 @@
>  #!/bin/sh
>  
> +RSYSLOGD_ARGS=""
> +ENABLED="yes"
> +
> +# shellcheck source=/dev/null
> +[ -r /etc/default/logging ] && . /etc/default/logging

 I think we should use /etc/default/rsyslog here. The options there are not at
all compatible with the syslogd/klogd options, so there is no point making it
the same file. That way, you can also use $DAEMON here.

 Actually, the script itself should really be called S01rsyslog. Only that is
not possible, because then busybox would install its own S01logging and we start
two syslog daemons.  But that's a special case, in general we'd like the init
script to be called S??$DAEMON.

> +
> +DAEMON="rsyslogd"
> +
> +test "$ENABLED" = "yes" || {
> +	printf '%s is disabled\n' "$DAEMON"
> +	exit 0
> +}
> +
>  start() {
> -  printf "Starting rsyslog daemon: "
> -  start-stop-daemon -S -q -p /var/run/rsyslogd.pid --exec /usr/sbin/rsyslogd
> -  [ $? = 0 ] && echo "OK" || echo "FAIL"
> +	printf 'Starting %s: ' "$DAEMON"
> +	{
> +		# shellcheck disable=SC2086 # we need the word splitting
> +		start-stop-daemon -b -S -q -p /var/run/rsyslogd.pid -x /usr/sbin/rsyslogd -- -n $RSYSLOGD_ARGS && \

 To make it more generic, use /var/run/${DAEMON}.pid and /usr/sbin/${DAEMON}.
Although for the latter I'm not entirely convinced it's a good idea. Similarly,
we could call the variable DAEMON_ARGS, but again I'm not convinced it's a good
idea.

 Oh, and +1 to Nicolas' comments.

 Regards,
 Arnout

[snip]

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [PATCH 5/8] sysklogd: update S01logging

[Arnout Vandecappelle]

On 09-07-18 05:31, Carlos Santos wrote:
> +		start-stop-daemon -b -S -q -p /var/run/syslogd.pid -x /sbin/syslogd -- -n $SYSLOGD_ARGS && \
> +		start-stop-daemon -b -S -q -p /var/run/klogd.pid -x /sbin/klogd -- -n $KLOGD_ARGS && \

 Just like rsyslogd, I believe syslogd and klogd can background and create a PID
file themselves. In that case, it's better not to use start-stop-daemon's -b option.

 Note that this is different for systemd units, because systemd does actual
monitoring of the processes it starts; it can only do that if the process
doesn't background itself. In sysvinit, however, there is no monitoring so it's
better if daemons daemonize themselves.

 Regards,
 Arnout
-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [PATCH 6/8] sysklogd: add logging configuration file

[Arnout Vandecappelle]

On 09-07-18 05:31, Carlos Santos wrote:
> +# Use KLOGD_RELOAD to select the "S01logging reload" behavior.
> +# - "USR1" will cause the kernel module symbols to be reloaded.
> +# - "USR2" will cause both the static kernel symbols and the kernel module
> +#   symbols to be reloaded.
> +# - "0" will do nothing.
> +# - Do not use any other value here. Use kill(1) for fine-grained control of
> +#   klogd, as documented in its manual page.
> +# KLOGD_RELOAD="0" # (default value)

 I don't really think it makes much sense to have this configurable. I don't see
any real need to ever do reloading of klogd in buildroot context (you're not
going to dynamically install kernel modules, are you?). In the rare cases that
this does happen, the user should be smart enough to send a SIGUSR1/2. Let's
just drop all this klogd reloading.

 Regards,
 Arnout

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [PATCH 7/8] rsyslog: update S01logging

[Arnout Vandecappelle]

 Note: subject is wrong s/rsyslog/syslog-ng/

On 09-07-18 05:31, Carlos Santos wrote:
> Reformat and fix syslog-ng startup script for better quality and code
> style:
> 
> - Indent with tabs, not spaces.
> - Use logic operators && and || to detect/handle errors, which provides
>   better readability than nested if/then/else blocks.
> - Use brackets for blocking, also improving readability.
> - Rewrite restart to report the result of the whole operation instead of
>   invoking stop, start and report OK twice.
> - Do not kill syslog-ng in "reload". Send a SIGHUP signal, instructing
>   it to perform a re-initialization.
> - Support a configuration file at /etc/default (an example file will be
>   added in forthcomming patch).
> - Support a configuration variable that completely disables the service
>   and issues a warning message on any invocation.
> 
> Signed-off-by: Carlos Santos <casantos@datacom.com.br>
> ---
>  package/syslog-ng/S01logging | 83 +++++++++++++++++++++++++-----------
>  1 file changed, 58 insertions(+), 25 deletions(-)
> 
> diff --git a/package/syslog-ng/S01logging b/package/syslog-ng/S01logging
> index d7c899a1e3..ba2eb226c1 100644
> --- a/package/syslog-ng/S01logging
> +++ b/package/syslog-ng/S01logging
> @@ -1,38 +1,71 @@
>  #!/bin/sh
>  
> +SYSLOG_NG_ARGS=""
> +ENABLED="yes"
> +
> +# shellcheck source=/dev/null
> +[ -r /etc/default/logging ] && . /etc/default/logging
> +
> +DAEMON="syslog-ng"
> +
> +test "$ENABLED" = "yes" || {
> +	printf '%s is disabled\n' "$DAEMON"
> +	exit 0
> +}
> +
>  start() {
> -	printf "Starting syslog-ng daemon: "
> -	start-stop-daemon -S -q -p /var/run/syslog-ng.pid \
> -		-x /usr/sbin/syslog-ng -- --pidfile /var/run/syslog-ng.pid
> -	[ $? = 0 ] && echo "OK" || echo "FAIL"
> +	printf 'Starting %s: ' "$DAEMON"
> +	{
> +		# shellcheck disable=SC2086 # we need the word splitting
> +		start-stop-daemon -b -S -q -p /var/run/syslog-ng.pid -x /usr/sbin/syslog-ng -- -F $SYSLOG_NG_ARGS && \

 Again, better let the daemon take care of backgrounding itself.

 Regards,
 Arnout


[snip]
-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [PATCH 3/8] rsyslog: update S01logging

[Carlos Santos]

> From: "Nicolas Cavallari" <Nicolas.Cavallari@green-communications.fr>
> To: "DATACOM" <casantos@datacom.com.br>, "buildroot" <buildroot@buildroot.org>
> Cc: "ratbert90" <aduskett@gmail.com>
> Sent: Monday, July 9, 2018 5:03:37 AM
> Subject: Re: [Buildroot] [PATCH 3/8] rsyslog: update S01logging

> Hello,
> 
> On 09/07/2018 05:31, Carlos Santos wrote:
>> --- a/package/rsyslog/S01logging
>> +++ b/package/rsyslog/S01logging
>> @@ -1,36 +1,64 @@
>>  #!/bin/sh
>>  
>> +RSYSLOGD_ARGS=""
>> +ENABLED="yes"
>> +
>> +# shellcheck source=/dev/null
>> +[ -r /etc/default/logging ] && . /etc/default/logging
>> +
>> +DAEMON="rsyslogd"
>> +
>> +test "$ENABLED" = "yes" || {
> 
> I find it strange to use test here and use [ ] everywhere else.
> And why not use if here ?

The "[ command ] &&" sequence is less readable, IMO. I'm doing the same in all
scripts. 

>> +	printf '%s is disabled\n' "$DAEMON"
>> +	exit 0
>> +}
>> +
>>  start() {
>> -  printf "Starting rsyslog daemon: "
>> -  start-stop-daemon -S -q -p /var/run/rsyslogd.pid --exec /usr/sbin/rsyslogd
>> -  [ $? = 0 ] && echo "OK" || echo "FAIL"
>> +	printf 'Starting %s: ' "$DAEMON"
>> +	{
>> +		# shellcheck disable=SC2086 # we need the word splitting
>> +		start-stop-daemon -b -S -q -p /var/run/rsyslogd.pid -x /usr/sbin/rsyslogd --
>> -n $RSYSLOGD_ARGS && \
> 
> Why do you disable backgrounding in rsyslog and ask start-stop-daemon to do it
> instead ?

It prevents hanging the boot sequence by putting a seemingly innocent "-n"
in RSYSLOGD_ARGS (in /etc/default/logging). It's impossible to interrupted
the script with Ctrl-C because stdin is redirected to /dev/null and, since
logging is the first service to start, there is no way to access the system
remotely via SSH or telnet to kill rsyslogd.

> This may prevent rsyslogd to report errors and exit 1 before it daemonize
> itself.

It's a trade-off. I chose safety.

>> +		echo "OK"
>> +	} || {
>> +		echo "FAIL"
>> +		exit 1
>> +	}
>>  }
> 
> 
>>  restart() {
>> -  stop
>> -  sleep 1
>> -  start
>> +	printf '%s %s: ' "${1:-Restarting}" "$DAEMON"
>> +	{
>> +		# shellcheck disable=SC2086 # we need the word splitting
>> +		start-stop-daemon -K -q -p /var/run/rsyslogd.pid && \
>> +		sleep 1 && \
>> +		start-stop-daemon -b -S -q -p /var/run/rsyslogd.pid -x /usr/sbin/rsyslogd --
>> -n $RSYSLOGD_ARGS && \
> 
> This duplicates code, and the error reporting is less precise than before.  It
> will just say "FAIL" if the daemon is not running or fails to start.
> 
>> +		echo "OK"
>> +	} || {
>> +		echo "FAIL"
>> +		exit 1
>> +	}
>> +}
>> +
>> +# rsyslogd ignores SIGHUP, SIGUSRx, so simply restart.
> 
> Are you sure about that ? the man page says otherwise.

I will update the comment to clarify that rsyslogd does not restart on
SIGHUP, just closes all open files. 

-- 
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 7/8] rsyslog: update S01logging

[Carlos Santos]

> From: "Arnout Vandecappelle" <arnout@mind.be>
> To: "DATACOM" <casantos@datacom.com.br>, "buildroot" <buildroot@buildroot.org>
> Cc: "ratbert90" <aduskett@gmail.com>
> Sent: Monday, July 9, 2018 7:05:30 PM
> Subject: Re: [Buildroot] [PATCH 7/8] rsyslog: update S01logging

> Note: subject is wrong s/rsyslog/syslog-ng/

OK.

[...]

>> +		# shellcheck disable=SC2086 # we need the word splitting
>> +		start-stop-daemon -b -S -q -p /var/run/syslog-ng.pid -x /usr/sbin/syslog-ng
>> -- -F $SYSLOG_NG_ARGS && \
> 
> Again, better let the daemon take care of backgrounding itself.
> 

As explained in the answer about rsyslogd, it prevents hanging the boot by
putting an "-F" in SYSLOG_NG_ARGS (in /etc/default/logging).

-- 
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 3/8] rsyslog: update S01logging

[Arnout Vandecappelle]

On 10-07-18 01:31, Carlos Santos wrote:
>> From: "Nicolas Cavallari" <Nicolas.Cavallari@green-communications.fr>
>> To: "DATACOM" <casantos@datacom.com.br>, "buildroot" <buildroot@buildroot.org>
>> Cc: "ratbert90" <aduskett@gmail.com>
>> Sent: Monday, July 9, 2018 5:03:37 AM
>> Subject: Re: [Buildroot] [PATCH 3/8] rsyslog: update S01logging
> 
>> Hello,
>>
>> On 09/07/2018 05:31, Carlos Santos wrote:
>>> --- a/package/rsyslog/S01logging
>>> +++ b/package/rsyslog/S01logging
>>> @@ -1,36 +1,64 @@
>>>  #!/bin/sh
>>>  
>>> +RSYSLOGD_ARGS=""
>>> +ENABLED="yes"
>>> +
>>> +# shellcheck source=/dev/null
>>> +[ -r /etc/default/logging ] && . /etc/default/logging
>>> +
>>> +DAEMON="rsyslogd"
>>> +
>>> +test "$ENABLED" = "yes" || {
>>
>> I find it strange to use test here and use [ ] everywhere else.
>> And why not use if here ?
> 
> The "[ command ] &&" sequence is less readable, IMO. I'm doing the same in all
> scripts. 

 If it is about readability: the most readable for is

if [ -z "$ENABLED" ]; then
  ...
fi

which is also the form we almost always use in our .mk files. We have about 40
occurences of 'if []', about 10 of 'if test', and exactly two of 'test ... &&'
and also two of '[ ... ] && ...' (but on the latter two, my counts could be wrong).


>>> +	printf '%s is disabled\n' "$DAEMON"
>>> +	exit 0
>>> +}
>>> +
>>>  start() {
>>> -  printf "Starting rsyslog daemon: "
>>> -  start-stop-daemon -S -q -p /var/run/rsyslogd.pid --exec /usr/sbin/rsyslogd
>>> -  [ $? = 0 ] && echo "OK" || echo "FAIL"
>>> +	printf 'Starting %s: ' "$DAEMON"
>>> +	{
>>> +		# shellcheck disable=SC2086 # we need the word splitting
>>> +		start-stop-daemon -b -S -q -p /var/run/rsyslogd.pid -x /usr/sbin/rsyslogd --
>>> -n $RSYSLOGD_ARGS && \
>>
>> Why do you disable backgrounding in rsyslog and ask start-stop-daemon to do it
>> instead ?
> 
> It prevents hanging the boot sequence by putting a seemingly innocent "-n"
> in RSYSLOGD_ARGS (in /etc/default/logging). It's impossible to interrupted
> the script with Ctrl-C because stdin is redirected to /dev/null and, since
> logging is the first service to start, there is no way to access the system
> remotely via SSH or telnet to kill rsyslogd.

 I don't agree with this reasoning. First of all, setting RSYSLOGD_ARGS is
already pretty advanced stuff, you can assume that people don't just randomly
put arguments in there. Second, you can easily put a "Do NOT use -n" sentence in
the documentation of RSYSLOGD_ARGS. Third, in the buildroot context, we can
assume that people will test the build before deploying to something that
doesn't allow easy recovery in case the system doesn't boot. There would maybe
be a case to be made if we had previously supported a defaults file where
RSYSLOGD_ARGS="-n" would do something sensible, but that is not the case.

 Actually, the latter could have been a reason to do this: _if_ we would
previously have a FOO_ARGS variable where it was reasonable to put a
do-not-daemonize argument in that variable, then we should think twice before
changing that behaviour. But AFAICS, the only FOO_ARGS we had was in busybox
syslogd, and that one will have to keep the -n option.

> 
>> This may prevent rsyslogd to report errors and exit 1 before it daemonize
>> itself.
> 
> It's a trade-off. I chose safety.
> 
>>> +		echo "OK"
>>> +	} || {
>>> +		echo "FAIL"
>>> +		exit 1
>>> +	}
>>>  }
>>
>>
>>>  restart() {
>>> -  stop
>>> -  sleep 1
>>> -  start
>>> +	printf '%s %s: ' "${1:-Restarting}" "$DAEMON"
>>> +	{
>>> +		# shellcheck disable=SC2086 # we need the word splitting
>>> +		start-stop-daemon -K -q -p /var/run/rsyslogd.pid && \
>>> +		sleep 1 && \
>>> +		start-stop-daemon -b -S -q -p /var/run/rsyslogd.pid -x /usr/sbin/rsyslogd --
>>> -n $RSYSLOGD_ARGS && \
>>
>> This duplicates code, and the error reporting is less precise than before.  It
>> will just say "FAIL" if the daemon is not running or fails to start.
>>
>>> +		echo "OK"
>>> +	} || {
>>> +		echo "FAIL"
>>> +		exit 1
>>> +	}
>>> +}
>>> +
>>> +# rsyslogd ignores SIGHUP, SIGUSRx, so simply restart.
>>
>> Are you sure about that ? the man page says otherwise.
> 
> I will update the comment to clarify that rsyslogd does not restart on
> SIGHUP, just closes all open files. 

 The important thing is: does rsyslogd reload its config file on SIGHUP? That is
the intended behaviour of 'reload': after changing daemon's config file, make
sure that the daemon uses the new config file. So for daemons that do monitoring
of their config files (e.g. using inotify), 'reload' should in fact not do anything.

 Regards,
 Arnout

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [PATCH 0/8] init scripts: rewrite S01logging

[Yann E. MORIN]

Carlos, All,

On 2018-07-09 00:31 -0300, Carlos Santos spake thusly:
> Continuing our effort to improve daemon startup scripts. This series
> focuses on S01logging, which starts the logging daemon.

Nice to see someone tackling the issue! :-)

I don't have much more to say than what Arnout and Nicolas already said,
i.e.:

  - use if-then-else blocks rather than the {}||{} construct;

  - allow restarting a stopped process;

  - restart and reload can be different: restart is stop-n-start, while
    reload should just instruct the daemon to reload its configuration.
    IFF the dameon does not handle reload, then reload should behave
    the same as restart;

  - starting an already started daemon is a noop, but is not an error;

  - don't try to protexct against stupidity in configuration files:
    users are expected to know what they will set in there. If they
    don't, they'll learn;

> Common features
> are:
> 
> - Indent with tabs, not spaces.
> - Implement start, stop, restart and reload as functions.
> - Use start-stop-daemon.
> - Use logic operators && and || to detect/handle errors, which provides
>   better readability than nested if/then/else blocks.
> - Use brackets for blocking, also improving readability.

Sorry, not that last two. if-then-else are very readable, and they are
very well knwon; while the {} || {} && {} constrcut is a bit convoluted
(that comment coming from the guy who's known for writing a *lot* of
big and complex shell scripts).

> - Correctly Detect and report start/stop/restart/reload errors.
> - Use separate functions for restart and reload; report the result of
>   the whole operations instead of invoking stop, start and report OK
>   twice.

I prefer that restart is an actual stop followed by an actual start. For
reload, it should, when supoprted by the daemon, ask it to just reload
its configuration' otherwise, reload is a restart.

Regards,
Yann E. MORIN.

> - Support a configuration file at /etc/default (example files for each
>   package will be added in separate patches).
> - Support a configuration variable that completely disables the service
>   and issues a warning message on any invocation.
> 
> The configuration files are provided mostly as examples for init script
> authors but they also contain information about options that cannot be
> used when running in background. 
> 
> All files implement the following FSM:
> 
>                                                   +---------+
>              +-------stop--------+   +----(1s)----| stopped |
>              |                   |   |            |   (*)   |
>              |                   |   |            +---------+
>              v                   |   v                 ^
>       +---------+             +---------+              |
>       |         |             |         |----restart---+
>       | STOPPED |----start--->| STARTED |
>       |         |             |         |----reload----+
>       +---------+             +---------+              |
>                                      ^                 |
>                                      |                 |
>                                      +-----------------+
> 
> * "stopped" is an intermediary state that transitions automatically to
>   STARTED after one second.
> 
> Attempts to do invalid transitions (e.g. start from STARTED state) will
> fail. That's why we don't pass -o (--oknodo) to start-stop-daemon. This
> changes the script behavior, in some cases, while in other cases just
> reports errors that were ignored previously.
> 
> We could interpret "start from STARTED" as "do nothing, ok" to match
> systemctl (systemd). Leave such change for later, however, since the
> current behavior matches most existing scripts.
> 
> Carlos Santos (8):
>   busybox: update S01logging
>   busybox: add logging configuration file
>   rsyslog: update S01logging
>   rsyslog: add logging configuration file
>   sysklogd: update S01logging
>   sysklogd: add logging configuration file
>   rsyslog: update S01logging
>   syslog-ng: add logging configuration file
> 
>  package/busybox/S01logging            | 87 +++++++++++++++++--------
>  package/busybox/busybox.mk            |  2 +
>  package/busybox/etc.default.logging   | 13 ++++
>  package/rsyslog/S01logging            | 74 ++++++++++++++-------
>  package/rsyslog/etc.default.logging   | 14 ++++
>  package/rsyslog/rsyslog.mk            |  2 +
>  package/sysklogd/S01logging           | 93 +++++++++++++++++++++------
>  package/sysklogd/etc.default.logging  | 25 +++++++
>  package/sysklogd/sysklogd.mk          |  2 +
>  package/syslog-ng/S01logging          | 83 +++++++++++++++++-------
>  package/syslog-ng/etc.default.logging | 14 ++++
>  package/syslog-ng/syslog-ng.mk        |  2 +
>  12 files changed, 316 insertions(+), 95 deletions(-)
>  create mode 100644 package/busybox/etc.default.logging
>  create mode 100644 package/rsyslog/etc.default.logging
>  create mode 100644 package/sysklogd/etc.default.logging
>  create mode 100644 package/syslog-ng/etc.default.logging
> 
> -- 
> 2.17.1
> 
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'