Date: Fri, 1 Nov 2013 12:06:17 +0100
From: Natanael Copa <ncopa@alpinelinux.org>
To: IT Offshore <developer@it-offshore.co.uk>
Cc: alpine-devel@lists.alpinelinux.org
Subject: Re: [alpine-devel] [PATCH] Initial APKBUILD file of PSAD (Port Scan
 Attack Detector)
Message-ID: <20131101120617.66fb3d44@ncopa-desktop.alpinelinux.org>
In-Reply-To: <1383277644-5024-1-git-send-email-developer@it-offshore.co.uk>
References: <1383277644-5024-1-git-send-email-developer@it-offshore.co.uk>
Precedence: list
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Fri,  1 Nov 2013 03:47:24 +0000
IT Offshore <developer@it-offshore.co.uk> wrote:

Thanks for the patch. Some comments on the init.d script.

> +++ b/testing/psad/psad.initd
> @@ -0,0 +1,221 @@
> +#!/sbin/runscript
> +
> +# This file is part of PSAD (Port Scan Attack Detector)
> +# Adapted for Alpine Linux by IT Offshore <developer@it-offshore.co.uk>
> +# Original Author: Franck Joncourt <franck@debian.org>
> +
> +PATH=/sbin:/usr/sbin:/bin:/usr/bin
> +DESC="Port Scan Attack Detector"
> +NAME=psad
> +DAEMON=/usr/sbin/$NAME
> +PIDDIR=/var/run/psad
> +SCRIPTNAME=/etc/init.d/psad
> +
> +depend() {
> +	need net
> +	need logger
> +	after iptables
> +}
> +
> +# Exit if the package is not installed
> +[ -x "$DAEMON" ] || exit 0

This should be removed. openrc will source the init.d scripts to parse dependencies. Unexpected things will happen if it exits early due to the binary file is missing.

> +
> +# Load user options to pass to psad daemon
> +DAEMON_ARGS=""
> +[ -r /etc/conf.d/psad ] && . /etc/conf.d/psad

This is not needed. runscript will read /etc/conf.d/$SVCNAME for you.

> +
> +# Function that checks if all of the configuration files exist
> +#
> +# Return
> +#   0 : all of the configuration files exist
> +#   6 : at least one file is missing
> +
> +check_config()
> +{
> +	local retval
> +	local file_list
> +
> +	retval=0
> +	file_list="/etc/psad/psad.conf"
> +
> +	for ConfFile in $file_list; do
> +		if [ ! -f "$ConfFile" ]; then
> +			retval=6	
> +		 	break	
> +		fi
> +	done
> +
> +	return $retval
> +}

I think its unecessary long function for a single config file. I'd do something like:

# allow override config_file location from conf.d
: ${config_file:="/etc/psad/psad.conf"}

check_config() {
	if ! [ -f "$config_file" ]; then
		error "$config_file is missing"
		return 1
	fi
}


> +
> +#
> +# Function to check if psad is running
> +#
> +#    0 : the psad.pid file has been found ; we assume the daemon is running
> +#    1 : no pid file has been found ; we assume the daemon is not running
> +#
> +is_psad_running()
> +{
> +        local pidfile="$PIDDIR/psad.pid"
> +        local retval
> +
> +        retval=0
> +        if [ -r "$pidfile" ]; then
> +                retval=1
> +        fi
> +
> +        return $retval
> +}

This function should not be needed. start-stop-daemon[1] can check if
pidfile exists. (Please note that openrc implementation of
start-stop-daemon is somewhat different from debians)

> +
> +#
> +# Function that starts the daemon/service
> +#
> +#   0 : daemon has been started or was already running
> +#   1 : generic or unspecified errors (could not be started)
> +#   6 : program is not configured (missing configuration files)
> +
> +do_start()
> +{
> +	local retval
> +
> +
> +        mkdir -p $PIDDIR
> +        chmod 755 $PIDDIR
> +
> +	# Check psad configuration
> +	check_config
> +	retval=$?
> +
> +	# Try to start psad
> +        is_psad_running
> +        if [ "$?" = 1 ]; then
> +            log_action_msg "The psad daemon is already running"
> +            retval=0
> +
> +	elif [ "$retval"  = "0" ]; then
> +		start-stop-daemon --start --quiet --pidfile $PIDDIR/$NAME --exec $DAEMON -- $DAEMON_ARGS
> +		retval="$?"
> +	fi
> +
> +	# Handle return status codes
> +	case "$retval" in
> +		0)	 
> +			;;
> +		6)	
> +			log_action_msg "You are missing the configuration file $ConfFile" || true
> +			;;
> +		9)	
> +			retval=0
> +			;;
> +		*)
> +			retval=1
> +			log_action_msg "Unable to start the daemon" || true
> +			;;
> +	esac
> +
> +	log_daemon_msg "Starting Port Scan Attack Detector" "psad" || true
> +        log_end_msg $retval || true
> +
> +	return $retval
> +}

This also looks way overcomplicated. i believe ebegin/eend should be
used instead of log_action_msg/log_daemon_msg.

> +
> +#
> +# Function that stops the daemon/service
> +#
> +# The upstream author has allowed the daemon to be killed through the 
> +# following command-line : psad --Kill
> +#
> +# As psad starts kmsgsd and psadwatchd on its own, we need to stop them before.
> +#
> +# Return
> +#   0 : daemon has been stopped or was already stopped
> +#   1 : daemon could not be stopped
> +
> +do_stop()
> +{
> +	local retval="0"
> +	local status kill_status
> +	local pid pidfile
> +	local process_list="psadwatchd kmsgsd psad"
> +
> +	# For each process
> +	for process in $process_list; do
> +
> +		pidfile="$PIDDIR/$process.pid"
> +		status="0"
> +		kill_status="1"
> +
> +		log_action_msg "Stopping the $process process"
> +
> +		# Try to kill the process associated to the pid
> +		if [ -r "$pidfile" ]; then
> +			pid=`cat "$pidfile" 2>/dev/null`
> +			kill -0 "${pid:-}" 2>/dev/null
> +			kill_status="$?"
> +		fi
> +
> +		# Stop the process
> +		if [ "$kill_status" = "0" ]; then
> +			start-stop-daemon --stop --oknodo --quiet --pidfile "$pidfile"
> +			status="$?"
> +		fi
> +
> +		# Remove its pid file
> +		if [ -r "$pidfile" ] && [ "$status" = "0" ]; then
> +			 rm -f "$pidfile" 2>/dev/null
> +			 status="$?"
> +		fi
> +
> +		[ "$status" = "0" ] || retval="1"
> +
> +	done
> +
> +	if [ "$retval" != "0" ]; then
> +		log_action_msg "One or more process could not be stopped" || true
> +	fi
> +
> +        log_daemon_msg "Stopping Port Scan Attack Detector" "psad" || true
> +        log_end_msg $retval || true
> +
> +	return $retval
> +}

runscript has logic that should make most of that code uneccessary.

> +
> +#
> +# Function that returns the daemon status
> +#
> +do_status()
> +{
> +	echo "Status of $DESC:"
> +	$DAEMON --Status
> +}

runscript does this automatic.

> +
> +case "$1" in
> +	start)
> +		do_start
> +		;;
> +
> +	stop)
> +		do_stop
> +		;;
> +
> +	restart|force-reload)
> +		do_stop
> +		sleep 1
> +		do_start
> +		;;
> +
> +	status)
> +		do_status
> +		exit $?
> +		;;
> +
> +	*)
> +		log_success_msg "Usage: $0 {start|stop|restart|status}" >&2
> +		exit 1 
> +		;;
> +esac
> +
> +exit

runscript does this too for you.

I believe the entire init.d script could be rewritten as:

---[BEGIN psad.initd]--------------------------------
#!/sbin/runscript

command="/usr/sbin/psad"
pidfile="/var/run/psad/psad.pid"

config_file="/etc/psad/psad.conf"

check_config() {
	[ -f "$config_file" ] || error "$config_file is missing"
}

start_pre() {
	check_config || return 1
	# make sure dir for pidfile exists. /var/run is tmpfs...
	checkpath --directory ${pidfile%/*}
}

---[END psad.initd]------------------------------------

runscript will take care of the rest.

the conf.d file could use the runscript's default command_args:

---[BEGIN psad.confd]----------------------------------
# Add any options you would like to pass to the daemon when started
# For example if you would like to add an override file for your setup, this
# can be achived this way:
#
#     command_args="--Override-config /root/psad.override.conf"
command_args=""
---[END psad.confd]-------------------------------------

For more info look at:
http://www.linuxhowtos.org/manpages/8/runscript.htm
http://wiki.alpinelinux.org/wiki/Writing_Init_Scripts

-nc

[1] http://linuxreviews.org/man/start-stop-daemon/


---
Unsubscribe:  alpine-devel+unsubscribe@lists.alpinelinux.org
Help:         alpine-devel+help@lists.alpinelinux.org
---