init scripts: rewrite S01logging
cover init scripts: rewrite S01logging
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
Signature · 1 line
2.17.1
PATCH 1/8 busybox: update S01logging
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.
--- package/busybox/S01logging | 87 ++++++++++++++++++++++++++------------ 1 file changed, 59 insertions(+), 28 deletions(-)
package/busybox/S01logging +59 -28
@@ -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 $?
Signature · 1 line
2.17.1
PATCH 1/8 busybox: update S01logging
Quoted 6 lines · Carlos Santos
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
Quoted 9 lines
package/busybox/S01logging +9
-0+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 && \
package/busybox/S01logging line 62
Quoted 10 lines
package/busybox/S01logging +10
-0+ 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.
Quoted 1 line
}
PATCH 1/8 busybox: update S01logging
Quoted 10 lines · Nicolas Cavallari
Quoted 6 lines · Carlos Santos
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...
Quoted 11 lines
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...
Quoted 15 lines
Quoted 15 lines
package/busybox/S01logging +14
-0+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 + }
Quoted 8 lines
package/busybox/S01logging line 62
Quoted 5 lines
package/busybox/S01logging +5
-0+} + +# 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
Signature · 6 lines
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
PATCH 1/8 busybox: update S01logging
In reply to Carlos SantosMore bikeshedding :-)
package/busybox/S01logging line 10
Quoted 5 lines · Carlos Santos
package/busybox/S01logging +3
-0+ +# 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.
package/busybox/S01logging line 18
Quoted 2 lines
package/busybox/S01logging +2
-0+# 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.
Quoted 4 lines
start(){package/busybox/S01logging +0
-3- 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
package/busybox/S01logging line 28
Quoted 10 lines
package/busybox/S01logging +9
-1- 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).
package/busybox/S01logging line 36
Quoted 12 lines
package/busybox/S01logging +5
-4+ } } 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]
Signature · 6 lines
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
PATCH 2/8 busybox: add logging configuration file
In reply to Carlos SantosProvide 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.
--- package/busybox/busybox.mk | 2 ++ package/busybox/etc.default.logging | 13 +++++++++++++ 2 files changed, 15 insertions(+) create mode 100644 package/busybox/etc.default.logging
package/busybox/busybox.mk +2 -0
@@ -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
package/busybox/etc.default.logging new file +13 -0
@@ -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"
Signature · 1 line
2.17.1
PATCH 3/8 rsyslog: update S01logging
In reply to Carlos SantosReformat 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.
--- package/rsyslog/S01logging | 74 ++++++++++++++++++++++++++------------ 1 file changed, 51 insertions(+), 23 deletions(-)
package/rsyslog/S01logging +51 -23
@@ -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 $?
Signature · 1 line
2.17.1
PATCH 3/8 rsyslog: update S01logging
Hello,
Quoted 4 lines · Carlos Santos
package/rsyslog/S01logging +0
-0@@ -1,36 +1,64 @@ #!/bin/sh
package/rsyslog/S01logging line 11
Quoted 10 lines
package/rsyslog/S01logging +9
-0+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 ?
package/rsyslog/S01logging line 20
Quoted 12 lines
package/rsyslog/S01logging +8
-3+ 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.
Quoted 8 lines
package/rsyslog/S01logging +5
-0+ echo "OK" + } || { + echo "FAIL" + exit 1 + } }
package/rsyslog/S01logging line 45
Quoted 10 lines
package/rsyslog/S01logging +6
-3restart() { - 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.
package/rsyslog/S01logging line 53
Quoted 8 lines
package/rsyslog/S01logging +8
-0+ echo "OK" + } || { + echo "FAIL" + exit 1 + } +} + +# rsyslogd ignores SIGHUP, SIGUSRx, so simply restart.
Are you sure about that ? the man page says otherwise.
PATCH 3/8 rsyslog: update S01logging
Quoted 14 lines
From: "Nicolas Cavallari" <[email protected]>
To: "DATACOM" <[email protected]>, "buildroot" <[email protected]>
Cc: "ratbert90" <[email protected]>
Sent: Monday, July 9, 2018 5:03:37 AM
Subject: Re: [Buildroot] [PATCH 3/8] rsyslog: update S01loggingHello,
Quoted 5 lines · Carlos Santos
package/rsyslog/S01logging +0
-0@@ -1,36 +1,64 @@ #!/bin/sh
Quoted 12 lines
package/rsyslog/S01logging line 11
Quoted 9 lines
package/rsyslog/S01logging +9
-0+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.
Quoted 8 lines
Quoted 8 lines
package/rsyslog/S01logging +4
-3+ 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"
Quoted 8 lines
package/rsyslog/S01logging
Quoted 5 lines
package/rsyslog/S01logging +4
-1+ 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.
Quoted 2 lines
This may prevent rsyslogd to report errors and exit 1 before it daemonize itself.
It's a trade-off. I chose safety.
Quoted 23 lines
Quoted 9 lines
package/rsyslog/S01logging +5
-0+ echo "OK" + } || { + echo "FAIL" + exit 1 + } } restart() {package/rsyslog/S01logging
Quoted 10 lines
package/rsyslog/S01logging +6
-4- 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.
Quoted 10 lines
package/rsyslog/S01logging line 53
Quoted 8 lines
package/rsyslog/S01logging +8
-0+ 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.
Signature · 3 lines
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
PATCH 3/8 rsyslog: update S01logging
Quoted 27 lines · Carlos Santos
Quoted 14 lines
From: "Nicolas Cavallari" <[email protected]>
To: "DATACOM" <[email protected]>, "buildroot" <[email protected]>
Cc: "ratbert90" <[email protected]>
Sent: Monday, July 9, 2018 5:03:37 AM
Subject: Re: [Buildroot] [PATCH 3/8] rsyslog: update S01loggingHello,
Quoted 5 lines · Carlos Santos
package/rsyslog/S01logging +0
-0@@ -1,36 +1,64 @@ #!/bin/shQuoted 12 lines
package/rsyslog/S01logging line 11
Quoted 9 lines
package/rsyslog/S01logging +9
-0+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 ?
Quoted 2 lines
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).
Quoted 17 lines
Quoted 8 lines
Quoted 8 lines
package/rsyslog/S01logging +4
-3+ 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"Quoted 8 lines
package/rsyslog/S01logging
Quoted 5 lines
package/rsyslog/S01logging +4
-1+ 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 ?
Quoted 5 lines
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/nulland, 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.
Quoted 40 lines
Quoted 3 lines
This may prevent rsyslogd to report errors and exit 1 before it daemonize itself.
It's a trade-off. I chose safety.
Quoted 23 lines
Quoted 9 lines
package/rsyslog/S01logging +5
-0+ echo "OK" + } || { + echo "FAIL" + exit 1 + } } restart() {package/rsyslog/S01logging
Quoted 10 lines
package/rsyslog/S01logging +6
-4- 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.
Quoted 10 lines
package/rsyslog/S01logging line 53
Quoted 8 lines
package/rsyslog/S01logging +8
-0+ echo "OK" + } || { + echo "FAIL" + exit 1 + } +} + +# rsyslogd ignores SIGHUP, SIGUSRx, so simply restart.Are you sure about that ? the man page says otherwise.
Quoted 2 lines
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
Signature · 6 lines
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
PATCH 3/8 rsyslog: update S01logging
In reply to Carlos SantosQuoted 26 lines · 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 <[email protected]>--- package/rsyslog/S01logging | 74 ++++++++++++++++++++++++++------------ 1 file changed, 51 insertions(+), 23 deletions(-)package/rsyslog/S01logging +0
-0
package/rsyslog/S01logging line 7
Quoted 7 lines
package/rsyslog/S01logging +5
-0@@ -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.
Quoted 6 lines
package/rsyslog/S01logging +6
-0+ +DAEMON="rsyslogd" + +test "$ENABLED" = "yes" || { + printf '%s is disabled\n' "$DAEMON" + exit 0
package/rsyslog/S01logging line 20
Quoted 10 lines
package/rsyslog/S01logging +6
-3+} + 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]
Signature · 6 lines
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
PATCH 4/8 rsyslog: add logging configuration file
In reply to Carlos SantosProvide 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.
--- package/rsyslog/etc.default.logging | 14 ++++++++++++++ package/rsyslog/rsyslog.mk | 2 ++ 2 files changed, 16 insertions(+) create mode 100644 package/rsyslog/etc.default.logging
package/rsyslog/etc.default.logging new file +14 -0
@@ -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"
package/rsyslog/rsyslog.mk +2 -0
@@ -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
Signature · 1 line
2.17.1
PATCH 5/8 sysklogd: update S01logging
In reply to Carlos SantosReformat 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.
--- package/sysklogd/S01logging | 93 +++++++++++++++++++++++++++++-------- 1 file changed, 74 insertions(+), 19 deletions(-)
package/sysklogd/S01logging +74 -19
@@ -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
Signature · 1 line
2.17.1
PATCH 5/8 sysklogd: update S01logging
Review
Quoted 2 lines · Carlos Santos
(quoted diff) +2
-0+ 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
Signature · 6 lines
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
PATCH 6/8 sysklogd: add logging configuration file
In reply to Carlos SantosProvide 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.
--- package/sysklogd/etc.default.logging | 25 +++++++++++++++++++++++++ package/sysklogd/sysklogd.mk | 2 ++ 2 files changed, 27 insertions(+) create mode 100644 package/sysklogd/etc.default.logging
package/sysklogd/etc.default.logging new file +25 -0
@@ -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"
package/sysklogd/sysklogd.mk +2 -0
@@ -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
Signature · 1 line
2.17.1
PATCH 6/8 sysklogd: add logging configuration file
package/sysklogd/etc.default.logging line 22
Quoted 8 lines · Carlos Santos
package/sysklogd/etc.default.logging new file +8
-0+# 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
Signature · 6 lines
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
PATCH 7/8 rsyslog: update S01logging
In reply to Carlos SantosReformat 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.
--- package/syslog-ng/S01logging | 83 +++++++++++++++++++++++++----------- 1 file changed, 58 insertions(+), 25 deletions(-)
package/syslog-ng/S01logging +58 -25
@@ -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
Signature · 1 line
2.17.1
PATCH 7/8 rsyslog: update S01logging
Note: subject is wrong s/rsyslog/syslog-ng/
Quoted 40 lines · 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 <[email protected]>--- package/syslog-ng/S01logging | 83 +++++++++++++++++++++++++----------- 1 file changed, 58 insertions(+), 25 deletions(-)package/syslog-ng/S01logging +12
-0@@ -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 +}
package/syslog-ng/S01logging line 20
Quoted 10 lines
package/syslog-ng/S01logging +5
-4+ 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]
Signature · 6 lines
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
PATCH 7/8 rsyslog: update S01logging
Quoted 7 lines
From: "Arnout Vandecappelle" <[email protected]>
To: "DATACOM" <[email protected]>, "buildroot" <[email protected]>
Cc: "ratbert90" <[email protected]>
Sent: Monday, July 9, 2018 7:05:30 PM
Subject: Re: [Buildroot] [PATCH 7/8] rsyslog: update S01loggingNote: subject is wrong s/rsyslog/syslog-ng/
OK.
[...]
Quoted 6 lines
Review
Quoted 3 lines
(quoted diff) +2
-1+ # 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).
Signature · 3 lines
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
PATCH 8/8 syslog-ng: add logging configuration file
In reply to Carlos SantosProvide 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.
--- 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
package/syslog-ng/etc.default.logging new file +14 -0
@@ -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"
package/syslog-ng/syslog-ng.mk +2 -0
@@ -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
Signature · 1 line
2.17.1
cover init scripts: rewrite S01logging
In reply to Carlos SantosHi 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.
Quoted 2 lines · Carlos Santos
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!
Quoted 3 lines
are:
- Indent with tabs, not spaces.
+1.
Quoted 1 line
- Implement start, stop, restart and reload as functions.
+1.
Quoted 1 line
- 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.
Quoted 2 lines
- 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.
Quoted 1 line
- Use brackets for blocking, also improving readability.
... Proving my point: with if, you get blocking automatically.
Quoted 1 line
- 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.
Quoted 3 lines
- 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.
Quoted 2 lines
- 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.
Quoted 2 lines
- 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.
Quoted 3 lines
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.
Quoted 16 lines
All files implement the following FSM:
+---------+ +-------stop--------+ +----(1s)----| stopped | | | | | (*) | | | | +---------+ v | v ^ +---------+ +---------+ | | | | |----restart---+ | STOPPED |----start--->| STARTED | | | | |----reload----+ +---------+ +---------+ | ^ | | | +-----------------+
Quoted 2 lines
* "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.
Quoted 4 lines
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).
Quoted 3 lines
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
Quoted 29 lines
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 filepackage/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
Signature · 6 lines
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
cover init scripts: rewrite S01logging
In reply to Carlos SantosCarlos, All,
On 2018-07-09 00:31 -0300, Carlos Santos spake thusly:
Quoted 2 lines
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;
Quoted 9 lines
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).
Quoted 4 lines
- 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.
Quoted 72 lines
- 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 filepackage/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.loggingSignature · 6 lines
2.17.1 _______________________________________________ buildroot mailing list buildroot at busybox.net http://lists.busybox.net/mailman/listinfo/buildroot
Signature · 6 lines
.-----------------.--------------------.------------------.--------------------. | 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. | '------------------------------^-------^------------------^--------------------'