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

[Buildroot] [PATCH v3 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.
- Correctly detect and report start/stop/restart/reload errors.
- Support a configuration file at /etc/default (example files for each
  package are provided in individual 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 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.

The "restart" transition is implemented as "stop, sleep 1, start", so
restarting from STOPPED state is possible, although an error message is
shown because "stop" fails.

The "reload" transition semantics is "reload the configuration and keep
running", when possible, otherwise it is the same as "restart" (in this
series, only syslog-ng supports a true "reload" operation).

The scripts and configuration files checked with shellcheck v0.5.0:

  $ shellcheck -s sh package/*/{S01logging,etc.default.logging}

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
  syslog-ng: update S01logging
  syslog-ng: add logging configuration file

 package/busybox/S01logging            | 82 +++++++++++++++---------
 package/busybox/busybox.mk            |  2 +
 package/busybox/etc.default.logging   | 17 +++++
 package/rsyslog/S01logging            | 68 +++++++++++++-------
 package/rsyslog/etc.default.logging   | 24 +++++++
 package/rsyslog/rsyslog.mk            |  2 +
 package/sysklogd/S01logging           | 90 +++++++++++++++++++++------
 package/sysklogd/etc.default.logging  | 33 ++++++++++
 package/sysklogd/sysklogd.mk          |  2 +
 package/syslog-ng/S01logging          | 73 +++++++++++++++-------
 package/syslog-ng/etc.default.logging | 21 +++++++
 package/syslog-ng/syslog-ng.mk        |  2 +
 12 files changed, 323 insertions(+), 93 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 v3 1/8] busybox: update S01logging

[Carlos Santos]

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

- Detect and report start/stop errors (previous version ignored them and
  always reported OK).
- Use a separate function for restart.
- Implement reload as restart.
- 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>
Reviewed-by: Matt Weber <matthew.weber@rockwellcollins.com>
---
Changes v1->v2
- Implement suggestions made by Nicolas Cavallari and Arnout Vandecappelle
---
 package/busybox/S01logging | 82 +++++++++++++++++++++++++-------------
 1 file changed, 54 insertions(+), 28 deletions(-)

diff --git a/package/busybox/S01logging b/package/busybox/S01logging
index fcb3e7d236..ddd27bba9a 100644
--- a/package/busybox/S01logging
+++ b/package/busybox/S01logging
@@ -1,40 +1,66 @@
 #!/bin/sh
-#
-# Start logging
-#
 
-SYSLOGD_ARGS=-n
-KLOGD_ARGS=-n
-[ -r /etc/default/logging ] && . /etc/default/logging
+DAEMON="logging"
+SPIDFILE="/var/run/syslogd.pid"
+KPIDFILE="/var/run/klogd.pid"
 
+SYSLOGD_ARGS=""
+KLOGD_ARGS=""
+ENABLED="yes"
+
+# shellcheck source=/dev/null
+[ -r "/etc/default/$DAEMON" ] && . "/etc/default/$DAEMON"
+
+if [ "$ENABLED" != "yes" ]; then
+	printf '%s is disabled\n' "$DAEMON"
+	exit 0
+fi
+
+# BusyBox' syslogd and klogd do not create pidfiles, so use "-m" to instruct
+# start-stop-daemon to create them. This also means that we must pass "-n" to
+# sylogd and klogd in the 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"
+	status=0
+	# shellcheck disable=SC2086 # we need the word splitting
+	start-stop-daemon -b -S -q -m -p "$SPIDFILE" -x /sbin/syslogd -- -n $SYSLOGD_ARGS || status=$?
+	start-stop-daemon -b -S -q -m -p "$KPIDFILE" -x /sbin/klogd -- -n $KLOGD_ARGS || status=$?
+	if [ "$status" -eq 0 ]; then
+		echo "OK"
+	else
+		echo "FAIL"
+	fi
+	return "$status"
 }
 
 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"
+	status=0
+	start-stop-daemon -K -q -p "$SPIDFILE" || status=$?
+	[ "$status" -eq 0 ] && rm -f "$SPIDFILE"
+	start-stop-daemon -K -q -p "$KPIDFILE" || status=$?
+	[ "$status" -eq 0 ] && rm -f "$KPIDFILE"
+	if [ "$status" -eq 0 ]; then
+		echo "OK"
+	else
+		echo "FAIL"
+	fi
+	return "$status"
 }
 
-case "$1" in
-  start)
-	start
-	;;
-  stop)
-	stop
-	;;
-  restart|reload)
+restart() {
 	stop
+	sleep 1
 	start
-	;;
-  *)
-	echo "Usage: $0 {start|stop|restart|reload}"
-	exit 1
-esac
+}
 
-exit $?
+case "$1" in
+        start|stop|restart)
+		"$1";;
+	reload)
+		# Restart, since there is no true "reload" feature.
+		restart;;
+        *)
+                echo "Usage: $0 {start|stop|restart|reload}"
+                exit 1
+esac
-- 
2.17.1

[Buildroot] [PATCH v3 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>
---
Changes v1->v2
- Implement suggestions made by Nicolas Cavallari and Arnout Vandecappelle
Changes v2->v3
- Add reference to documentation, as suggested by Matt Weber.
---
 package/busybox/busybox.mk          |  2 ++
 package/busybox/etc.default.logging | 17 +++++++++++++++++
 2 files changed, 19 insertions(+)
 create mode 100644 package/busybox/etc.default.logging

diff --git a/package/busybox/busybox.mk b/package/busybox/busybox.mk
index 757086592f..91131c0012 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..5e52eff985
--- /dev/null
+++ b/package/busybox/etc.default.logging
@@ -0,0 +1,17 @@
+#
+# /etc/default/logging (busybox version)
+#
+# Online documentation of syslogd and klogd is available at
+#
+#    https://busybox.net/downloads/BusyBox.html
+#
+
+# 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 v3 3/8] rsyslog: update S01logging

[Carlos Santos]

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

- Indent with tabs, not spaces.
- 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>
---
Changes v1->v2
- Implement suggestions made by Nicolas Cavallari and Arnout Vandecappelle
Changes v2->v3
- Include /etc/default/logging, not /etc/default/$DAEMON.
---
 package/rsyslog/S01logging | 68 +++++++++++++++++++++++++-------------
 1 file changed, 45 insertions(+), 23 deletions(-)

diff --git a/package/rsyslog/S01logging b/package/rsyslog/S01logging
index 8e4a59c2d5..5764780467 100644
--- a/package/rsyslog/S01logging
+++ b/package/rsyslog/S01logging
@@ -1,36 +1,58 @@
 #!/bin/sh
 
+DAEMON="rsyslogd"
+PIDFILE="/var/run/$DAEMON.pid"
+
+RSYSLOGD_ARGS=""
+ENABLED="yes"
+
+# shellcheck source=/dev/null
+[ -r "/etc/default/logging" ] && . "/etc/default/logging"
+
+if [ "$ENABLED" != "yes" ]; then
+	printf '%s is disabled\n' "$DAEMON"
+	exit 0
+fi
+
 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 -S -q -p "$PIDFILE" -x /usr/sbin/rsyslogd -- $RSYSLOGD_ARGS
+	status=$?
+	if [ "$status" -eq 0 ]; then
+		echo "OK"
+	else
+		echo "FAIL"
+	fi
+	return "$status"
 }
 
 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 "$PIDFILE"
+	status=$?
+	if [ "$status" -eq 0 ]; then
+		echo "OK"
+	else
+		echo "FAIL"
+	fi
+	return "$status"
 }
 
 restart() {
-  stop
-  sleep 1
-  start
+	stop
+	sleep 1
+	start
 }
 
 case "$1" in
-  start)
-    start
-    ;;
-  stop)
-    stop
-    ;;
-  restart|reload)
-    restart
-    ;;
-  *)
-    echo "Usage: $0 {start|stop|restart}"
-    exit 1
+        start|stop|restart)
+		"$1";;
+	reload)
+		# Restart, since there is no true "reload" feature (does not
+		# reconfigure/restart on SIGHUP, just closes all open files).
+		restart;;
+        *)
+                echo "Usage: $0 {start|stop|restart|reload}"
+                exit 1
 esac
-
-exit $?
-- 
2.17.1

[Buildroot] [PATCH v3 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>
---
Changes v1->v2
- Document that "-n" command line option must not be used.
- Small improvements in comments.
Changes v2->v3
- Add reference to documentation, as suggested by Matt Weber.
---
 package/rsyslog/etc.default.logging | 24 ++++++++++++++++++++++++
 package/rsyslog/rsyslog.mk          |  2 ++
 2 files changed, 26 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..12223eba21
--- /dev/null
+++ b/package/rsyslog/etc.default.logging
@@ -0,0 +1,24 @@
+#
+# /etc/default/logging (rsyslog version)
+#
+# Online documentation of rsyslog is available at
+#
+#    https://www.rsyslog.com/doc/
+#
+# Command line options are mostly undocumented in recent rsyslog versions, so
+# avoid using them. Only "-n" and "-d" are briefly described when rsyslogd is
+# invoked with the "-h" option. Other options are described on old versions of
+# the rsyslogd(8) manual page but they are certainly outdated.
+#
+
+# Use RSYSLOGD_ARGS to pass additional arguments to rsyslogd.
+# - Do NOT use "-i pid_file", "-n", nor "-v", since they will break the
+#   startup script.
+# - Use "-d" to enable debug (see below). Warning: log files may be 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 v3 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.
- Detect and report start/stop errors (previous version ignored them and
  always reported OK).
- 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.

Signed-off-by: Carlos Santos <casantos@datacom.com.br>
---
Changes v1->v2
- Implement suggestions made by Arnout Vandecappelle
Changes v2->v3
- Include /etc/default/logging, not /etc/default/$DAEMON.
---
 package/sysklogd/S01logging | 90 ++++++++++++++++++++++++++++---------
 1 file changed, 70 insertions(+), 20 deletions(-)

diff --git a/package/sysklogd/S01logging b/package/sysklogd/S01logging
index 1cbfe869fa..9b6e879d8a 100644
--- a/package/sysklogd/S01logging
+++ b/package/sysklogd/S01logging
@@ -1,25 +1,75 @@
 #!/bin/sh
 
-case "$1" in
-	start)
-		printf "Starting logging: "
-		/sbin/syslogd -m 0
-		/sbin/klogd
+DAEMON="sysklogd"
+SPIDFILE="/var/run/syslogd.pid"
+KPIDFILE="/var/run/klogd.pid"
+
+SYSLOGD_ARGS="-m 0"
+KLOGD_ARGS=""
+KLOGD_RELOAD="0"
+ENABLED="yes"
+
+# shellcheck source=/dev/null
+[ -r "/etc/default/logging" ] && . "/etc/default/logging"
+
+if [ "$ENABLED" != "yes" ]; then
+	printf '%s is disabled\n' "$DAEMON"
+	exit 0
+fi
+
+start() {
+	printf 'Starting %s: ' "$DAEMON"
+	status=0
+	# shellcheck disable=SC2086 # we need the word splitting
+	start-stop-daemon -S -q -p "$SPIDFILE" -x /sbin/syslogd -- $SYSLOGD_ARGS || status=$?
+	start-stop-daemon -S -q -p "$KPIDFILE" -x /sbin/klogd -- $KLOGD_ARGS || status=$?
+	if [ "$status" -eq 0 ]; then
+		echo "OK"
+	else
+		echo "FAIL"
+	fi
+	return "$status"
+}
+
+stop() {
+	printf 'Stopping %s: ' "$DAEMON"
+	status=0
+	start-stop-daemon -K -q -p "$SPIDFILE" || status=$?
+	start-stop-daemon -K -q -p "$KPIDFILE" || status=$?
+	if [ "$status" -eq 0 ]; then
 		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`
+	else
+		echo "FAIL"
+	fi
+	return "$status"
+}
+
+restart() {
+	stop
+	sleep 1
+	start
+}
+
+# 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"
+	status=0
+	start-stop-daemon -K -s HUP -q -p "$SPIDFILE" || status=$?
+	start-stop-daemon -K -s "$KLOGD_RELOAD" -q -p "$KPIDFILE" || status=$?
+	if [ "$status" -eq 0 ]; then
 		echo "OK"
-		;;
-	restart|reload)
-		$0 stop
-		$0 start
-		;;
-	*)
-		echo "Usage: $0 {start|stop|restart}"
-		exit 1
-esac
+	else
+		echo "FAIL"
+	fi
+	return "$status"
+}
 
-exit $?
+case "$1" in
+        start|stop|restart|reload)
+                "$1";;
+        *)
+                echo "Usage: $0 {start|stop|restart|reload}"
+                exit 1
+esac
-- 
2.17.1

[Buildroot] [PATCH v3 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>
---
Changes v1->v2:
- Document that the "-n" command line option must not be used.
- Going against the request from Arnout Vandecappelle, keep the klogd reload
  configuration. Add a comment explainig why it is still useful. BTW, I work
  on a project which requires dynamic installation of kernel modules.
Changes v2->v3
- Add reference to documentation, as suggested by Matt Weber.
---
 package/sysklogd/etc.default.logging | 33 ++++++++++++++++++++++++++++
 package/sysklogd/sysklogd.mk         |  2 ++
 2 files changed, 35 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..a655e7ad42
--- /dev/null
+++ b/package/sysklogd/etc.default.logging
@@ -0,0 +1,33 @@
+#
+# /etc/default/logging (sysklogd version)
+#
+# Documentation is available in the klogd.8 and sysklogd.8 manual pages
+# distributed along with the source code.
+#
+
+# Use SYSLOGD_ARGS to pass additional arguments to syslogd.
+# - Do NOT use "-n", "-v", since they 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 "-n", "-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)
+
+# KLOGD_RELOAD selects 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.
+#
+# Most users will never need to reload klogd in buildroot context because they
+# are not going to dynamically install kernel modules. In the rare cases that
+# this does happen, the user should be smart enough to send a SIGUSR1/2 but
+# let's keep this configurable for documentation purposes.
+# 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 v3 7/8] syslog-ng: update S01logging

[Carlos Santos]

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

- Indent with tabs, not spaces.
- 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>
---
Changes v1->v2:
- Implement changes suggested by Arnout Vandecappelle.
Changes v2->v3:
- Include /etc/default/logging, not /etc/default/$DAEMON.
- Remove stray 'g' spotted by Chris Packham.
---
 package/syslog-ng/S01logging | 73 +++++++++++++++++++++++++-----------
 1 file changed, 51 insertions(+), 22 deletions(-)

diff --git a/package/syslog-ng/S01logging b/package/syslog-ng/S01logging
index d7c899a1e3..255ca16660 100644
--- a/package/syslog-ng/S01logging
+++ b/package/syslog-ng/S01logging
@@ -1,17 +1,42 @@
 #!/bin/sh
 
+DAEMON="syslog-ng"
+PIDFILE="/var/run/$DAEMON.pid"
+
+SYSLOG_NG_ARGS=""
+ENABLED="yes"
+
+# shellcheck source=/dev/null
+[ -r "/etc/default/logging" ] && . "/etc/default/logging"
+
+if [ "$ENABLED" != "yes" ]; then
+	printf '%s is disabled\n' "$DAEMON"
+	exit 0
+fi
+
 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 -S -q -p "$PIDFILE" -x /usr/sbin/syslog-ng -- $SYSLOG_NG_ARGS
+	status=$?
+	if [ "$status" -eq 0 ]; then
+		echo "OK"
+	else
+		echo "FAIL"
+	fi
+	return "$status"
 }
 
 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 "$PIDFILE"
+	status=$?
+	if [ "$status" -eq 0 ]; then
+		echo "OK"
+	else
+		echo "FAIL"
+	fi
+	return "$status"
 }
 
 restart() {
@@ -20,19 +45,23 @@ restart() {
 	start
 }
 
+# SIGHUP makes syslog-ng reload its configuration
+reload() {
+	printf 'Stopping %s: ' "$DAEMON"
+	start-stop-daemon -K -s HUP -q -p "$PIDFILE"
+	status=$?
+	if [ "$status" -eq 0 ]; then
+		echo "OK"
+	else
+		echo "FAIL"
+	fi
+	return "$status"
+}
+
 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 v3 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>
---
Changes v1->v2
- Document that -F/--foreground must not be used
Changes v2->v3:
- Add reference to documentation, as suggested by Matt Weber.
- Fix copy/paste error spotted by Chris Packham (s/rsyslog/syslog-ng/).
---
 package/syslog-ng/etc.default.logging | 21 +++++++++++++++++++++
 package/syslog-ng/syslog-ng.mk        |  2 ++
 2 files changed, 23 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..f0f4d52b02
--- /dev/null
+++ b/package/syslog-ng/etc.default.logging
@@ -0,0 +1,21 @@
+#
+# /etc/default/logging (syslog-ng version)
+#
+# Documentation is available in the syslog-ng.8 and syslog-ng.conf.5 manual
+# pages distributed along with the source code, in the doc/man subdirectory.
+#
+# Online documentation of syslog-ng is available at
+#
+#    https://www.syslog-ng.com/technical-documents/
+#
+
+# Use SYSLOG_NG_ARGS to pass additional arguments to syslog-ng.
+# - Do NOT use "-F/--foreground", "--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 v3 1/8] busybox: update S01logging

[Matthew Weber]

Carlos,

On Sun, Oct 7, 2018 at 6:46 AM Carlos Santos <casantos@datacom.com.br> wrote:
>
> Reformat and fix syslogd/klogd startup script for better quality and
> code style:
>
> - Detect and report start/stop errors (previous version ignored them and
>   always reported OK).
> - Use a separate function for restart.
> - Implement reload as restart.
> - 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>
> Reviewed-by: Matt Weber <matthew.weber@rockwellcollins.com>

Tested-by: Matt Weber <matthew.weber@rockwellcollins.com>

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

[Matthew Weber]

Carlos,

On Sun, Oct 7, 2018 at 6:46 AM Carlos Santos <casantos@datacom.com.br> wrote:
>
> 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>

Tested the disable option in the config and the init script picked it
up as expected.

Tested-by: Matt Weber <matthew.weber@rockwellcollins.com>

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

[Matthew Weber]

Carlos,

On Sun, Oct 7, 2018 at 6:46 AM Carlos Santos <casantos@datacom.com.br> wrote:
>
> Reformat and fix rsyslog startup script for better quality and code
> style:
>
> - Indent with tabs, not spaces.
> - 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>

Tested-by: Matt Weber <matthew.weber@rockwellcollins.com>

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

[Matthew Weber]

Carlos,

On Sun, Oct 7, 2018 at 6:46 AM Carlos Santos <casantos@datacom.com.br> wrote:
>
> 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>

Similar to the busybox config, I disabled the logger and the init
script reflected the change.

Tested-by: Matt Weber <matthew.weber@rockwellcollins.com>

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

[Thomas Petazzoni]

Hello,

On Sun,  7 Oct 2018 08:45:57 -0300, Carlos Santos wrote:
> 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.
> - Correctly detect and report start/stop/restart/reload errors.
> - Support a configuration file at /etc/default (example files for each
>   package are provided in individual patches).
> - Support a configuration variable that completely disables the service
>   and issues a warning message on any invocation.

Thanks a lot for working on this, much appreciated. Overall, I am happy
with the proposal you're making and the consistency between init
scripts that your proposal is bringing.

The only thing that I'm not entirely sure about is specific to these
logging init scripts:

 (1) Do we really want that Busybox, syslog-ng, rsyslog, sysklogd all
     install a file of the same name, S01logging ? Isn't this
     contradicting our goal of not having one package overwrite files
     installed by another package ?

     Wouldn't it make more sense to install S01syslog-ng, S01rsyslog,
     S01sysklogd ? This way, we also have a mapping between the init
     script name and the daemon/package being started ?

 (2) For the Busybox case, do we want to keep a single init script that
     starts both klogd and syslogd ? Or should we have one init script
     per daemon, so that again the init script name matches the daemon
     being started ? So S01klogd, S01syslogd ?

Note: the above points are really proposals/discussion items. Do not
rework your series according to those comments until there has been
some discussion/agreement with other developers.

Another point, but which can definitely be handled separately, is
documenting in the manual what the init scripts should look like, by at
least pointing to the "good" init scripts, so that people know which
ones are good to use as a template.

Thanks a lot!

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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

[Carlos Santos]

> From: "Thomas Petazzoni" <thomas.petazzoni@bootlin.com>
> To: "DATACOM" <casantos@datacom.com.br>
> Cc: "buildroot" <buildroot@buildroot.org>, "ratbert90" <aduskett@gmail.com>, "Arnout Vandecappelle" <arnout@mind.be>,
> "Peter Korsgaard" <peter@korsgaard.com>, "Yann Morin" <yann.morin.1998@free.fr>
> Sent: Thursday, October 11, 2018 12:09:48 PM
> Subject: Re: [Buildroot] [PATCH v3 0/8] init scripts: rewrite S01logging

> Hello,
> 
> On Sun,  7 Oct 2018 08:45:57 -0300, Carlos Santos wrote:
>> 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.
>> - Correctly detect and report start/stop/restart/reload errors.
>> - Support a configuration file at /etc/default (example files for each
>>   package are provided in individual patches).
>> - Support a configuration variable that completely disables the service
>>   and issues a warning message on any invocation.
> 
> Thanks a lot for working on this, much appreciated. Overall, I am happy
> with the proposal you're making and the consistency between init
> scripts that your proposal is bringing.
> 
> The only thing that I'm not entirely sure about is specific to these
> logging init scripts:
> 
> (1) Do we really want that Busybox, syslog-ng, rsyslog, sysklogd all
>     install a file of the same name, S01logging ? Isn't this
>     contradicting our goal of not having one package overwrite files
>     installed by another package ?
> 
>     Wouldn't it make more sense to install S01syslog-ng, S01rsyslog,
>     S01sysklogd ? This way, we also have a mapping between the init
>     script name and the daemon/package being started ?

I don't know why it was decided to use the same name for all packages.
Looks like the original intention was to prevent multiple logging damons
from running at the same time.

Perhaps we should add an item to the "System configuration" menu allowing
the user to choose a logging provider, just like the "Init system" item,
and convert rsyslog/sysklogd/syslog-ng to virtual packages.

> (2) For the Busybox case, do we want to keep a single init script that
>     starts both klogd and syslogd ? Or should we have one init script
>     per daemon, so that again the init script name matches the daemon
>     being started ? So S01klogd, S01syslogd ?

There is also sysklogd, from which Busyox borrowed the code (or at least
the idea) a long time ago. I think we should keep klogd and syslogd tied
to each other, since klogd does not make much sense without syslogd.

> Note: the above points are really proposals/discussion items. Do not
> rework your series according to those comments until there has been
> some discussion/agreement with other developers.
> 
> Another point, but which can definitely be handled separately, is
> documenting in the manual what the init scripts should look like, by at
> least pointing to the "good" init scripts, so that people know which
> ones are good to use as a template.

It's already in my endless to-do list. :-)

-- 
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 v3 0/8] init scripts: rewrite S01logging

[Thomas Petazzoni]

Hello,

On Fri, 12 Oct 2018 08:50:49 -0300 (BRT), Carlos Santos wrote:

> > (1) Do we really want that Busybox, syslog-ng, rsyslog, sysklogd all
> >     install a file of the same name, S01logging ? Isn't this
> >     contradicting our goal of not having one package overwrite files
> >     installed by another package ?
> > 
> >     Wouldn't it make more sense to install S01syslog-ng, S01rsyslog,
> >     S01sysklogd ? This way, we also have a mapping between the init
> >     script name and the daemon/package being started ?  
> 
> I don't know why it was decided to use the same name for all packages.
> Looks like the original intention was to prevent multiple logging damons
> from running at the same time.

Yes, but why? If we go down this route, then all web servers should
install S50httpd, and not S50lighttpd, S50apache, etc. Dropbear and
OpenSSH should not install S50dropbear and S50sshd, etc.

> Perhaps we should add an item to the "System configuration" menu allowing
> the user to choose a logging provider, just like the "Init system" item,
> and convert rsyslog/sysklogd/syslog-ng to virtual packages.

I don't think we want to do that, because then we need to do this for
"what mail server do you want?", "what http server do you want?", "what
SSH server do you want?", etc.

If you enable two SSH servers, the second to start will fail. If you
enable two HTTP servers, the second to start will fail, etc. Similarly,
if you enable two logging daemons, it will not work nicely.

I don't think we should solve that problem, and just leave it up to the
user to do a configuration that makes sense. If you enable two SSH
servers or two logging daemons, your configuration doesn't make sense,
and it should be fixed.

> > (2) For the Busybox case, do we want to keep a single init script that
> >     starts both klogd and syslogd ? Or should we have one init script
> >     per daemon, so that again the init script name matches the daemon
> >     being started ? So S01klogd, S01syslogd ?  
> 
> There is also sysklogd, from which Busyox borrowed the code (or at least
> the idea) a long time ago. I think we should keep klogd and syslogd tied
> to each other, since klogd does not make much sense without syslogd.

Tied to each other doesn't necessarily mean they should be handled by a
common init script. busybox.mk can install S01klogd S02syslogd, and we
keep this one daemon == one init script logic ?

Otherwise, assuming we stop using the common S01logging name, what
would be the name of the init script installed by Busybox for its
logging daemons ?

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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

[Arnout Vandecappelle]

 Hi Carlos,

 Thank you again for this series. I still have some feedback, which I discussed
as well with the others here at the Buildroot meeting, and we generally agree on
these aspects.

 Let me start with repeating that we appreciate very much this work.


On 07/10/2018 12:45, Carlos Santos wrote:
> Reformat and fix syslogd/klogd startup script for better quality and
> code style:
> 
> - Detect and report start/stop errors (previous version ignored them and
>   always reported OK).
> - Use a separate function for restart.
> - Implement reload as restart.
> - 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>
> Reviewed-by: Matt Weber <matthew.weber@rockwellcollins.com>
> ---
> Changes v1->v2
> - Implement suggestions made by Nicolas Cavallari and Arnout Vandecappelle
> ---
>  package/busybox/S01logging | 82 +++++++++++++++++++++++++-------------

 Thomas mentioned that he would prefer every init script to be called the same
as the executable it starts (which is usually the same as the package). There is
general agreement about that aspect.

 Also he mentioned that it is better to split into two separate scripts when two
daemons are started (i.e., klogd). There's agreement on that as well.

 Still, these changes can be done in follow-up patches, so it's OK if you keep
it as S01logging for the time being (if you want to do the split right away,
that's OK as well). However, the names of the default files would be better to
already reflect the daemon name rather than just /etc/default/logging.

 Note also that with the splitting, we can improve the situation for busybox a
little. Now, it is possible that klogd is NOT selected in busybox, but it is
still started. With a separate init script, it would be possible to install it
only when klogd is enabled (like is done now for syslogd).

>  1 file changed, 54 insertions(+), 28 deletions(-)
> 
> diff --git a/package/busybox/S01logging b/package/busybox/S01logging
> index fcb3e7d236..ddd27bba9a 100644
> --- a/package/busybox/S01logging
> +++ b/package/busybox/S01logging
> @@ -1,40 +1,66 @@
>  #!/bin/sh
> -#
> -# Start logging
> -#
>  
> -SYSLOGD_ARGS=-n
> -KLOGD_ARGS=-n
> -[ -r /etc/default/logging ] && . /etc/default/logging
> +DAEMON="logging"
> +SPIDFILE="/var/run/syslogd.pid"
> +KPIDFILE="/var/run/klogd.pid"
>  
> +SYSLOGD_ARGS=""
> +KLOGD_ARGS=""
> +ENABLED="yes"
> +
> +# shellcheck source=/dev/null
> +[ -r "/etc/default/$DAEMON" ] && . "/etc/default/$DAEMON> +
> +if [ "$ENABLED" != "yes" ]; then
> +	printf '%s is disabled\n' "$DAEMON"

 The ENABLED option is not useful, it is felt. If you don't want the daemon to
be started, just remove the init script or make it non-executable.

> +	exit 0
> +fi
> +
> +# BusyBox' syslogd and klogd do not create pidfiles, so use "-m" to instruct
> +# start-stop-daemon to create them. This also means that we must pass "-n" to
> +# sylogd and klogd in the 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"
> +	status=0
> +	# shellcheck disable=SC2086 # we need the word splitting
> +	start-stop-daemon -b -S -q -m -p "$SPIDFILE" -x /sbin/syslogd -- -n $SYSLOGD_ARGS || status=$?

 Small nit, personal preference of mine: I think it's clearer if the syslogd
args themselves are on a separate line, i.e.

	start-stop-daemon -b -S -q -m -p "$SPIDFILE" -x /sbin/syslogd \
		-- -n $SYSLOGD_ARGS || status=$?

> +	start-stop-daemon -b -S -q -m -p "$KPIDFILE" -x /sbin/klogd -- -n $KLOGD_ARGS || status=$?
> +	if [ "$status" -eq 0 ]; then

 There was some bikeshedding about needing quotes around $status here, but in
the end, it's not important, as long as it's consistent.

> +		echo "OK"
> +	else
> +		echo "FAIL"
> +	fi
> +	return "$status"
>  }
>  

 The rest LGTM. So basically two important comments: 1. remove ENABLED; 2. name
the defaults file like the daemon.	

 Regards,
 Arnout

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

[Arnout Vandecappelle]

On 07/10/2018 12:45, Carlos Santos wrote:
> 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.

 We also discussed this, and we agreed in the end that it is not very useful to
have a default defaults file. It needlessly consumes space on the target, the
user anyway probably has to consult documentation to know which options to use,
and there is (usually) only one option that can be set in the file:
<DAEMON>_ARGS. In addition, we can't expect all package authors to submit
something that is of such a high quality as this example...

 Regards,
 Arnout

> 
> Signed-off-by: Carlos Santos <casantos@datacom.com.br>
> ---
> Changes v1->v2
> - Implement suggestions made by Nicolas Cavallari and Arnout Vandecappelle
> Changes v2->v3
> - Add reference to documentation, as suggested by Matt Weber.
> ---
>  package/busybox/busybox.mk          |  2 ++
>  package/busybox/etc.default.logging | 17 +++++++++++++++++
>  2 files changed, 19 insertions(+)
>  create mode 100644 package/busybox/etc.default.logging
> 
> diff --git a/package/busybox/busybox.mk b/package/busybox/busybox.mk
> index 757086592f..91131c0012 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..5e52eff985
> --- /dev/null
> +++ b/package/busybox/etc.default.logging
> @@ -0,0 +1,17 @@
> +#
> +# /etc/default/logging (busybox version)
> +#
> +# Online documentation of syslogd and klogd is available at
> +#
> +#    https://busybox.net/downloads/BusyBox.html
> +#
> +
> +# 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"
> 

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

[Carlos Santos]

> From: "Arnout Vandecappelle" <arnout@mind.be>
> To: "DATACOM" <casantos@datacom.com.br>, "buildroot" <buildroot@buildroot.org>
> Cc: "ratbert90" <aduskett@gmail.com>, "Matthew Weber" <matthew.weber@rockwellcollins.com>, "Chris Packham"
> <judge.packham@gmail.com>
> Sent: Domingo, 21 de outubro de 2018 15:27:24
> Subject: Re: [PATCH v3 2/8] busybox: add logging configuration file

> On 07/10/2018 12:45, Carlos Santos wrote:
>> 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.
> 
> We also discussed this, and we agreed in the end that it is not very useful to
> have a default defaults file. It needlessly consumes space on the target, the
> user anyway probably has to consult documentation to know which options to use,
> and there is (usually) only one option that can be set in the file:
> <DAEMON>_ARGS. In addition, we can't expect all package authors to submit
> something that is of such a high quality as this example...

OK, I will drop the default files from the series.

-- 
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 v3 0/8] init scripts: rewrite S01logging

[Carlos Santos]

> From: "Thomas Petazzoni" <thomas.petazzoni@bootlin.com>
> To: "DATACOM" <casantos@datacom.com.br>
> Cc: "buildroot" <buildroot@buildroot.org>, "ratbert90" <aduskett@gmail.com>, "Arnout Vandecappelle" <arnout@mind.be>,
> "Peter Korsgaard" <peter@korsgaard.com>, "Yann Morin" <yann.morin.1998@free.fr>
> Sent: S?bado, 13 de outubro de 2018 9:55:12
> Subject: Re: [Buildroot] [PATCH v3 0/8] init scripts: rewrite S01logging

> Hello,
> 
> On Fri, 12 Oct 2018 08:50:49 -0300 (BRT), Carlos Santos wrote:
> 
>> > (1) Do we really want that Busybox, syslog-ng, rsyslog, sysklogd all
>> >     install a file of the same name, S01logging ? Isn't this
>> >     contradicting our goal of not having one package overwrite files
>> >     installed by another package ?
>> > 
>> >     Wouldn't it make more sense to install S01syslog-ng, S01rsyslog,
>> >     S01sysklogd ? This way, we also have a mapping between the init
>> >     script name and the daemon/package being started ?
>> 
>> I don't know why it was decided to use the same name for all packages.
>> Looks like the original intention was to prevent multiple logging damons
>> from running at the same time.
> 
> Yes, but why? If we go down this route, then all web servers should
> install S50httpd, and not S50lighttpd, S50apache, etc. Dropbear and
> OpenSSH should not install S50dropbear and S50sshd, etc.
> 
>> Perhaps we should add an item to the "System configuration" menu allowing
>> the user to choose a logging provider, just like the "Init system" item,
>> and convert rsyslog/sysklogd/syslog-ng to virtual packages.
> 
> I don't think we want to do that, because then we need to do this for
> "what mail server do you want?", "what http server do you want?", "what
> SSH server do you want?", etc.
> 
> If you enable two SSH servers, the second to start will fail. If you
> enable two HTTP servers, the second to start will fail, etc. Similarly,
> if you enable two logging daemons, it will not work nicely.
> 
> I don't think we should solve that problem, and just leave it up to the
> user to do a configuration that makes sense. If you enable two SSH
> servers or two logging daemons, your configuration doesn't make sense,
> and it should be fixed.

Did I ever suggest adding menus to select HTTP and SSH servers? Let's stick
to what was said, please.

>> > (2) For the Busybox case, do we want to keep a single init script that
>> >     starts both klogd and syslogd ? Or should we have one init script
>> >     per daemon, so that again the init script name matches the daemon
>> >     being started ? So S01klogd, S01syslogd ?
>> 
>> There is also sysklogd, from which Busyox borrowed the code (or at least
>> the idea) a long time ago. I think we should keep klogd and syslogd tied
>> to each other, since klogd does not make much sense without syslogd.
> 
> Tied to each other doesn't necessarily mean they should be handled by a
> common init script. busybox.mk can install S01klogd S02syslogd, and we
> keep this one daemon == one init script logic ?
> 
> Otherwise, assuming we stop using the common S01logging name, what
> would be the name of the init script installed by Busybox for its
> logging daemons ?

The problem here is Busybox. If we name each init script after the
corresponding daemon and select syslog-ng we will have both S01syslogd
and S01syslog-ng installed unless we have some configuration and/or
put some logic like this in busybox.mk:

ifeq ($(BR2_PACKAGE_SYSKLOGD)$(BR2_PACKAGE_RSYSLOG)$(BR2_PACKAGE_SYSLOG_NG),)
define BUSYBOX_INSTALL_LOGGING_SCRIPT
        if grep -q CONFIG_SYSLOGD=y $(@D)/.config; \
        then \
                $(INSTALL) -m 0755 -D package/busybox/S01syslogd \
                        $(TARGET_DIR)/etc/init.d/S01syslogd; \
        fi; \
        if grep -q CONFIG_KLOGD=y $(@D)/.config; \
        then \
                $(INSTALL) -m 0755 -D package/busybox/S02klogd \
                        $(TARGET_DIR)/etc/init.d/S02klogd; \
        fi
endef
endif

Is that OK for you?

-- 
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 v3 0/8] init scripts: rewrite S01logging

[Thomas Petazzoni]

Hello Carlos,

On Fri, 2 Nov 2018 19:13:49 -0200 (BRST), Carlos Santos wrote:

> >> Perhaps we should add an item to the "System configuration" menu allowing
> >> the user to choose a logging provider, just like the "Init system" item,
> >> and convert rsyslog/sysklogd/syslog-ng to virtual packages.  
> > 
> > I don't think we want to do that, because then we need to do this for
> > "what mail server do you want?", "what http server do you want?", "what
> > SSH server do you want?", etc.
> > 
> > If you enable two SSH servers, the second to start will fail. If you
> > enable two HTTP servers, the second to start will fail, etc. Similarly,
> > if you enable two logging daemons, it will not work nicely.
> > 
> > I don't think we should solve that problem, and just leave it up to the
> > user to do a configuration that makes sense. If you enable two SSH
> > servers or two logging daemons, your configuration doesn't make sense,
> > and it should be fixed.  
> 
> Did I ever suggest adding menus to select HTTP and SSH servers? Let's stick
> to what was said, please.

I'm not sure why you felt the need to become angry here, there really
was no such need. I was just commenting on why I thought adding a menu
for selecting the logging solution was IMO not appropriate, because it
was in my opinion a direction that could lead to doing the same for
tons of other packages that have conflicting functionality. So yes, the
comparison with HTTP and SSH servers made sense: I was not implying
*you* were suggesting to add menus for HTTP and SSH servers, but rather
that if we start adding more and more system configuration menus to
select between potentially conflicting packages, we may have people
proposing patches to select HTTP/SSH servers.

E-mail communication probably didn't communicate my feeling very well
here. Because overall, I am *really* happy with you taking the lead on
this init script cleanup discussion. It's long overdue, and I'm very
happy that someone like you has decided to do something about it,
knowing that doing something about it includes getting through the
usual bikeshedding that the Buildroot community is so good at.

So please take my comments in this discussion as nice comments from
someone who is really happy to see you help cleaning up our init
scripts! :-)

> The problem here is Busybox. If we name each init script after the
> corresponding daemon and select syslog-ng we will have both S01syslogd
> and S01syslog-ng installed unless we have some configuration and/or
> put some logic like this in busybox.mk:

To me, this is exactly like what happens if you enable both Dropbear
and OpenSSH. It installs two init scripts with different names, even if
in practice, only one SSH server will start. We don't handle this
"conflict" for the SSH servers, I don't see why we should have some
special thing to handle it for logging daemons.

> Is that OK for you?

Yes, I think this downside is OK for me, because it's a downside we
already have for lots of other packages in Buildroot, and fixing it
requires way too much complexity compared to the benefits.

Of course, this is only my opinion, and other BR developers can share
their possibly different view on this.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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

[Arnout Vandecappelle]

On 02/11/18 22:25, Thomas Petazzoni wrote:
> Hello Carlos,
> 
> On Fri, 2 Nov 2018 19:13:49 -0200 (BRST), Carlos Santos wrote:
> 
>>>> Perhaps we should add an item to the "System configuration" menu allowing
>>>> the user to choose a logging provider, just like the "Init system" item,
>>>> and convert rsyslog/sysklogd/syslog-ng to virtual packages.  
>>>
>>> I don't think we want to do that, because then we need to do this for
>>> "what mail server do you want?", "what http server do you want?", "what
>>> SSH server do you want?", etc.
>>>
>>> If you enable two SSH servers, the second to start will fail. If you
>>> enable two HTTP servers, the second to start will fail, etc. Similarly,
>>> if you enable two logging daemons, it will not work nicely.
>>>
>>> I don't think we should solve that problem, and just leave it up to the
>>> user to do a configuration that makes sense. If you enable two SSH
>>> servers or two logging daemons, your configuration doesn't make sense,
>>> and it should be fixed.  
[snip]
>> The problem here is Busybox. If we name each init script after the
>> corresponding daemon and select syslog-ng we will have both S01syslogd
>> and S01syslog-ng installed unless we have some configuration and/or
>> put some logic like this in busybox.mk:
> 
> To me, this is exactly like what happens if you enable both Dropbear
> and OpenSSH. It installs two init scripts with different names, even if
> in practice, only one SSH server will start. We don't handle this
> "conflict" for the SSH servers, I don't see why we should have some
> special thing to handle it for logging daemons.

 There is one big difference with the HTTP daemon, however: even if busybox
httpd is enabled, we don't install an init script for it. However, S01logging is
installed unconditionally, even if the user didn't consciously choose for
busybox as its logging provider.

[Note: there are three other init scripts that are installed implicitly by
busybox: mdev (which does have an explicit option), watchdog (but no non-busybox
equivalent exists for it) and telnet (but we have removed the standalone telnetd
package).

Note 2: a somewhat similar situation exists with S40networking, which
(probably?) conflicts with network-manager and connman.]

 In that sense, it does make sense to offer an option to choose between the
logging providers.


 That said, in general we prefer to do things implicitly by selecting a package
rather than having explicit choices for various system-level tools. Which indeed
leads us to:

>> ifeq ($(BR2_PACKAGE_SYSKLOGD)$(BR2_PACKAGE_RSYSLOG)$(BR2_PACKAGE_SYSLOG_NG),)
>> define BUSYBOX_INSTALL_LOGGING_SCRIPT
>>         if grep -q CONFIG_SYSLOGD=y $(@D)/.config; \
>>         then \
>>                 $(INSTALL) -m 0755 -D package/busybox/S01syslogd \
>>                         $(TARGET_DIR)/etc/init.d/S01syslogd; \
>>         fi; \

 Note that this bit was already there, but it also explicitly checked that
S01logging didn't exist. So the complexity of handling the conflict was already
there.

 Note also that it is no longer needed to add rsyslog and syslog-ng to the
busybox dependencies: they were only there because they install the S01logging
script which conflicts with the one installed by busybox. So in a way, doing it
this way is cleaner than what we had before.

>>         if grep -q CONFIG_KLOGD=y $(@D)/.config; \
>>         then \
>>                 $(INSTALL) -m 0755 -D package/busybox/S02klogd \
>>                         $(TARGET_DIR)/etc/init.d/S02klogd; \
>>         fi
>> endef
>> endif


> 
>> Is that OK for you?
> 
> Yes, I think this downside is OK for me, because it's a downside we
> already have for lots of other packages in Buildroot, and fixing it
> requires way too much complexity compared to the benefits.
> 
> Of course, this is only my opinion, and other BR developers can share
> their possibly different view on this.

 So yes, I agree with Thomas.


 However, as I mentioned before, I see this as completely orthogonal to the
S01logging cleanup, because this series is in fact mostly about standardizing
the format of the init scripts. So Carlos, if you prefer to keep it as
S01logging, that's fine.

 Thanks for your work on this!

 Regards,
 Arnout

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

[Thomas Petazzoni]

Hello,

On Fri, 2 Nov 2018 23:30:10 +0100, Arnout Vandecappelle wrote:

> > To me, this is exactly like what happens if you enable both Dropbear
> > and OpenSSH. It installs two init scripts with different names, even if
> > in practice, only one SSH server will start. We don't handle this
> > "conflict" for the SSH servers, I don't see why we should have some
> > special thing to handle it for logging daemons.  
> 
>  There is one big difference with the HTTP daemon, however: even if busybox
> httpd is enabled, we don't install an init script for it. However, S01logging is
> installed unconditionally, even if the user didn't consciously choose for
> busybox as its logging provider.

Well, my point was not simply for Busybox HTTP server, but for HTTP
servers in general. We have S50apache and S50lighttpd for example, and
possibly others as well. We have S50dropbear and S50sshd.

So the problem doesn't exist today for the Busybox HTTP server, because
we don't have an init script for it (but we may add one at some point
in the future). But it already exists for tons of other
daemons/services.

>  In that sense, it does make sense to offer an option to choose between the
> logging providers.

No, see above :-)

>  However, as I mentioned before, I see this as completely orthogonal to the
> S01logging cleanup, because this series is in fact mostly about standardizing
> the format of the init scripts. So Carlos, if you prefer to keep it as
> S01logging, that's fine.

On this, I totally agree, it is independent from Carlos work.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com