From 0bf07827dfb4116642bb8cfd0cc2be53b742c1d4 Mon Sep 17 00:00:00 2001 From: Michael Prokop Date: Thu, 21 May 2020 17:01:05 +0200 Subject: [PATCH] TT#82051 Shellcheck fixes + fix failure handling of init scripts shellcheck v0.7.1 complains about a bunch of issues: SC2154: status is referenced but not assigned. SC2181: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?. SC2207: Prefer mapfile or read -a to split command output (or quote to avoid splitting). SC2235: Use { ..; } instead of (..) to avoid subshell overhead. SC2236: Use -n instead of ! -z. The "$status" variable disappeared in d4763aba140d701f3323. The init scripts of ngcp-rtpengine-daemon and ngcp-rtpengine-recording-daemon had a logic bug, where a failing stop action didn't properly return, but continued execution of the following firewalling code (ngcp-rtpengine-iptables-setup). Thanks to Guillem Gover for spotting this one. While at it, no longer execute under 'set -e'. Change-Id: Ia50e76f615564a288627e6e42ec8f7eb082de74c --- debian/ngcp-rtpengine-daemon.init | 43 +++++++++++---------- debian/ngcp-rtpengine-daemon.postinst | 2 +- debian/ngcp-rtpengine-recording-daemon.init | 25 ++++++------ el/rtpengine.init | 15 +++---- 4 files changed, 43 insertions(+), 42 deletions(-) diff --git a/debian/ngcp-rtpengine-daemon.init b/debian/ngcp-rtpengine-daemon.init index 413f378a1..858544ead 100755 --- a/debian/ngcp-rtpengine-daemon.init +++ b/debian/ngcp-rtpengine-daemon.init @@ -9,8 +9,6 @@ # Description: Proxy for RTP and other media streams ### END INIT INFO -set -e - PATH=/sbin:/bin:/usr/sbin:/usr/bin NAME=ngcp-rtpengine-daemon DESC="RTP/media proxy" @@ -37,14 +35,14 @@ fi OPTIONS="" START_OPTIONS="" -if [ ! -z "$INTERFACES" ]; then +if [ -n "$INTERFACES" ]; then for interface in $INTERFACES; do OPTIONS="$OPTIONS --interface=$interface" done fi -if [ ! -z "$SUBSCRIBE_KEYSPACES" ]; then +if [ -n "$SUBSCRIBE_KEYSPACES" ]; then for ks in $SUBSCRIBE_KEYSPACES; do OPTIONS="$OPTIONS --subscribe-keyspace=$ks" done @@ -74,9 +72,9 @@ fi [ -z "$REDIS_NUM_THREADS" ] || OPTIONS="$OPTIONS --redis-num-threads=$REDIS_NUM_THREADS" [ -z "$REDIS_EXPIRES" ] || OPTIONS="$OPTIONS --redis-expires=$REDIS_EXPIRES" [ -z "$REDIS_MULTIKEY" ] || OPTIONS="$OPTIONS --redis-multikey=$REDIS_MULTIKEY" -[ -z "$NO_REDIS_REQUIRED" ] || ( [ "$NO_REDIS_REQUIRED" != "1" ] && [ "$NO_REDIS_REQUIRED" != "yes" ] ) || OPTIONS="$OPTIONS --no-redis-required" +[ -z "$NO_REDIS_REQUIRED" ] || { [ "$NO_REDIS_REQUIRED" != "1" ] && [ "$NO_REDIS_REQUIRED" != "yes" ] ; } || OPTIONS="$OPTIONS --no-redis-required" [ -z "$B2B_URL" ] || OPTIONS="$OPTIONS --b2b-url=$B2B_URL" -[ -z "$NO_FALLBACK" ] || ( [ "$NO_FALLBACK" != "1" ] && [ "$NO_FALLBACK" != "yes" ] ) || OPTIONS="$OPTIONS --no-fallback" +[ -z "$NO_FALLBACK" ] || { [ "$NO_FALLBACK" != "1" ] && [ "$NO_FALLBACK" != "yes" ] ; } || OPTIONS="$OPTIONS --no-fallback" OPTIONS="$OPTIONS --table=$TABLE" [ -z "$LOG_LEVEL" ] || OPTIONS="$OPTIONS --log-level=$LOG_LEVEL" [ -z "$LOG_FACILITY" ] || OPTIONS="$OPTIONS --log-facility=$LOG_FACILITY" @@ -91,7 +89,7 @@ OPTIONS="$OPTIONS --table=$TABLE" [ -z "$HOMER" ] || OPTIONS="$OPTIONS --homer=$HOMER" [ -z "$HOMER_PROTOCOL" ] || OPTIONS="$OPTIONS --homer-protocol=$HOMER_PROTOCOL" [ -z "$HOMER_ID" ] || OPTIONS="$OPTIONS --homer-id=$HOMER_ID" -if [ ! -z "$RECORDING_DIR" ]; then +if [ -n "$RECORDING_DIR" ]; then OPTIONS="$OPTIONS --recording-dir=$RECORDING_DIR" if [ ! -d "$RECORDING_DIR" ]; then mkdir "$RECORDING_DIR" 2> /dev/null @@ -100,7 +98,7 @@ if [ ! -z "$RECORDING_DIR" ]; then fi [ -z "$RECORDING_METHOD" ] || OPTIONS="$OPTIONS --recording-method=$RECORDING_METHOD" [ -z "$RECORDING_FORMAT" ] || OPTIONS="$OPTIONS --recording-format=$RECORDING_FORMAT" -[ -z "$DTLS_PASSIVE" ] || ( [ "$DTLS_PASSIVE" != "yes" ] && [ "$DTLS_PASSIVE" != "1" ] ) || OPTIONS="$OPTIONS --dtls-passive" +[ -z "$DTLS_PASSIVE" ] || { [ "$DTLS_PASSIVE" != "yes" ] && [ "$DTLS_PASSIVE" != "1" ] ; } || OPTIONS="$OPTIONS --dtls-passive" if test "$FORK" = "no" ; then OPTIONS="$OPTIONS --foreground" @@ -135,20 +133,26 @@ fi case "$1" in start) - set +e if [ -x "$(which ngcp-check-active)" ]; then case "$(ngcp-check-active -v)" in active|transition) log_action_msg "Active node or transition." ;; *) - log_action_msg "Ignored start action in inactive node ($status)" + log_action_msg "Ignored start action in inactive node" exit 0 ;; esac fi - ngcp-rtpengine-iptables-setup start - set -e + + RC=0 + ngcp-rtpengine-iptables-setup start || RC=$? + if [ "$RC" -ne 0 ]; then + log_action_msg "Failed to start ngcp-rtpengine-iptables-setup" + log_end_msg "$RC" + exit 1 + fi + log_daemon_msg "Starting $DESC" "$NAME" # shellcheck disable=SC2086 start-stop-daemon --start --quiet --pidfile "$PIDFILE" \ @@ -157,16 +161,15 @@ case "$1" in ;; stop) log_daemon_msg "Stopping $DESC" "$NAME" + RC=0 start-stop-daemon --oknodo --stop --quiet --pidfile $PIDFILE \ - --retry 5 --exec "$DAEMON" - if [ "$?" -ne 0 ]; then - return $? + --retry 5 --exec "$DAEMON" || RC=$? + if [ "$RC" -eq 0 ]; then + ngcp-rtpengine-iptables-setup stop || true fi - set +e - ngcp-rtpengine-iptables-setup stop - set -e - rm -f $PIDFILE - log_end_msg $? + rm -f "$PIDFILE" + log_end_msg "$RC" + exit "$RC" ;; restart|force-reload) $0 stop diff --git a/debian/ngcp-rtpengine-daemon.postinst b/debian/ngcp-rtpengine-daemon.postinst index b42e00f5a..5fe2ac8ff 100644 --- a/debian/ngcp-rtpengine-daemon.postinst +++ b/debian/ngcp-rtpengine-daemon.postinst @@ -20,7 +20,7 @@ else if [ -n "$TABLE" ] && [ "$TABLE" -ge 0 ] && \ [ -n "$NO_FALLBACK" ] && \ - ([ "$NO_FALLBACK" = "1" ] || [ "$NO_FALLBACK" = "yes" ]) + { [ "$NO_FALLBACK" = "1" ] || [ "$NO_FALLBACK" = "yes" ] ; } then if lsmod | grep -q $modname || modinfo $modname > /dev/null 2> /dev/null; then true diff --git a/debian/ngcp-rtpengine-recording-daemon.init b/debian/ngcp-rtpengine-recording-daemon.init index 79fdc603b..72ed03985 100755 --- a/debian/ngcp-rtpengine-recording-daemon.init +++ b/debian/ngcp-rtpengine-recording-daemon.init @@ -9,8 +9,6 @@ # Description: Recording daemon for RTP and other media streams ### END INIT INFO -set -e - PATH=/sbin:/bin:/usr/sbin:/usr/bin NAME=ngcp-rtpengine-recording-daemon DESC="RTP/media recording daemon" @@ -66,21 +64,25 @@ fi case "$1" in start) - set +e if [ -x "$(which ngcp-check-active)" ]; then case "$(ngcp-check-active -v)" in active|transition) log_action_msg "Active node or transition." ;; *) - log_action_msg "Ignored start action in inactive node ($status)" + log_action_msg "Ignored start action in inactive node" exit 0 ;; esac fi - set -e - ngcp-rtpengine-recording-nfs-setup start + RC=0 + ngcp-rtpengine-recording-nfs-setup start || RC=$? + if [ "$RC" -ne 0 ] ; then + log_action_msg "Failed to start ngcp-rtpengine-recording-nfs-setup" + log_end_msg "$RC" + exit "$RC" + fi log_daemon_msg "Starting $DESC" "$NAME" @@ -91,13 +93,12 @@ case "$1" in ;; stop) log_daemon_msg "Stopping $DESC" "$NAME" + RC=0 start-stop-daemon --oknodo --stop --quiet --pidfile $PIDFILE \ - --retry 5 --exec "$DAEMON" - if [ "$?" -ne 0 ]; then - return $? - fi - rm -f $PIDFILE - log_end_msg $? + --retry 5 --exec "$DAEMON" || RC=$? + rm -f "$PIDFILE" + log_end_msg "$RC" + exit "$RC" ;; force-reload|restart) $0 stop diff --git a/el/rtpengine.init b/el/rtpengine.init index 731a1fa00..45eeb68c7 100644 --- a/el/rtpengine.init +++ b/el/rtpengine.init @@ -109,7 +109,7 @@ build_opts() { [ -z "$REDIS_NUM_THREADS" ] || OPTS+=" --redis-num-threads=$REDIS_NUM_THREADS" [ -z "$REDIS_EXPIRES" ] || OPTS+=" --redis-expires=$REDIS_EXPIRES" [ -z "$REDIS_MULTIKEY" ] || OPTS+=" --redis-multikey=$REDIS_MULTIKEY" - [ -z "$NO_REDIS_REQUIRED" ] || ( [ "$NO_REDIS_REQUIRED" != "1" ] && [ "$NO_REDIS_REQUIRED" != "yes" ] ) || OPTS+=" --no-redis-required" + [ -z "$NO_REDIS_REQUIRED" ] || { [ "$NO_REDIS_REQUIRED" != "1" ] && [ "$NO_REDIS_REQUIRED" != "yes" ] ; } || OPTS+=" --no-redis-required" [ -z "$B2B_URL" ] || OPTS+=" --b2b-url=$B2B_URL" [ -z "$LOG_LEVEL" ] || OPTS+=" --log-level=$LOG_LEVEL" [ -z "$LOG_FACILITY" ] || OPTS+=" --log-facility=$LOG_FACILITY" @@ -126,10 +126,10 @@ build_opts() { [ -z "$HOMER_ID" ] || OPTS+=" --homer-id=$HOMER_ID" [ -z "$RECORDING_METHOD" ] || OPTS+=" --recording-method=$RECORDING_METHOD" [ -z "$RECORDING_FORMAT" ] || OPTS+=" --recording-format=$RECORDING_FORMAT" - [ -z "$DTLS_PASSIVE" ] || ( [ "$DTLS_PASSIVE" != "yes" ] && [ "$DTLS_PASSIVE" != "1" ] ) || OPTS+=" --dtls-passive" + [ -z "$DTLS_PASSIVE" ] || { [ "$DTLS_PASSIVE" != "yes" ] && [ "$DTLS_PASSIVE" != "1" ] ; } || OPTS+=" --dtls-passive" # recording dir - if [ ! -z "$RECORDING_DIR" ];then + if [ -n "$RECORDING_DIR" ];then OPTS+=" --recording-dir=$RECORDING_DIR" if [ ! -d "$RECORDING_DIR" ]; then mkdir "$RECORDING_DIR" 2>/dev/null @@ -154,12 +154,10 @@ start() { else modprobe xt_RTPENGINE fi - firewall-cmd --state 2>/dev/null - if [[ $? == 0 ]];then + if firewall-cmd --state 2>/dev/null ; then # Using firewalld # Need to check if the INPUT_prefilter chain is present (permanently) - firewall-cmd --permanent --direct --query-chain ipv4 filter INPUT_prefilter > /dev/null - if [[ $? != 0 ]];then + if ! firewall-cmd --permanent --direct --query-chain ipv4 filter INPUT_prefilter > /dev/null; then firewall-cmd --permanent --direct --add-chain ipv4 filter INPUT_prefilter firewall-cmd --permanent --direct --passthrough ipv4 -t filter -I INPUT -j INPUT_prefilter firewall-cmd --reload @@ -210,8 +208,7 @@ stop() { . "$cachefile" echo "Unloading module for in-kernel packet forwarding" echo "del $TABLE" > /proc/rtpengine/control - firewall-cmd --state 2>/dev/null - if [[ $? == 0 ]];then + if firewall-cmd --state 2>/dev/null; then firewall-cmd --direct --remove-rules ipv4 filter rtpengine firewall-cmd --direct --remove-rules ipv6 filter rtpengine firewall-cmd --direct --remove-rule ipv4 filter INPUT_prefilter 0 -j rtpengine