[Pkg-xen-devel] [PATCH 08/13] xen init script: rewrite xenstored start logic

[Ian Jackson]

Hans van Kranenburg writes ("[PATCH 08/13] xen init script: rewrite xenstored start logic"):
> -XENSTORED="$ROOT"/bin/xenstored
> +# In /etc/default/xen, the user can set XENSTORED, which has to be either
> +# 'xenstored' or 'oxenstored'. In here, we add the version specific path.
> +if [ -n "$XENSTORED" ]; then
> +	USER_XENSTORED="$ROOT"/bin/$XENSTORED

IMO it would be better to do this only if XENSTORED does not start
with /.  So maybe

   case "$XENSTORED" in
   /*) USER_XENSTORED="$XENSTORED" ;;
   ?*) USER_XENSTORED="$ROOT"/bin/$XENSTORED ;;
   '') USER_XENSTORED="" ;;
   esac

?

> +	# First, we check if any of xenstored or oxenstored is already running. If
> +	# so, we abort and leave it alone, even if the user has switched the value
> +	# in /etc/default/xen from one to another.
> +	for try_xenstored in "$OXENSTORED" "$CXENSTORED"; do
> +		if [ -x $try_xenstored ]; then
> +			start-stop-daemon --start --quiet --pidfile "$XENSTORED_PIDFILE" \
> +				--exec "$try_xenstored" --test > /dev/null
> +			if [ $? -eq 1 ]; then
> +				return 1

This exit status handling is confusing.  AFAICT from the manpage for
start-stop-daemon, exit status 1 means "--oknodo was not specified and
nothing was done".  So I think that would occur when --start does
nothing because the daemon is already running.  It might be worth
adding a comment.

> +	if [ -n "$USER_XENSTORED" ]; then
> +		if [ ! -x "$USER_XENSTORED" ]; then
> +			log_failure_msg "Failed to start $USER_XENSTORED: no such executable."
> +			return 2
> +		else
> +			start-stop-daemon --start --quiet \
> +				--pidfile "$XENSTORED_PIDFILE" --exec "$USER_XENSTORED" -- \
> +				$XENSTORED_ARGS --pid-file="$XENSTORED_PIDFILE"
> +			if [ $? -ne 0 ]; then
> +				return 2
> +			fi
> +		fi
> +	else
> +		for try_xenstored in "$OXENSTORED" "$CXENSTORED"; do
> +			if [ -x $try_xenstored ]; then
> +				start-stop-daemon --start --quiet \
> +					--pidfile "$XENSTORED_PIDFILE" --exec "$try_xenstored" -- \
> +					$XENSTORED_ARGS --pid-file="$XENSTORED_PIDFILE"

I really don't like the way all of this is open-coded.  There are two
almost-identical runes here.  How about this:

   case "$XENSTORED" in
   /*) USER_XENSTORED="$XENSTORED"           ; try_xenstoreds=USER_XENSTORED ;;
   ?*) USER_XENSTORED="$ROOT"/bin/$XENSTORED ; try_xenstoreds=USER_XENSTORED ;;
   '') USER_XENSTORED="" ; try_xenstoreds='OXENSTORED CXENSTORED' ;;
   esac

and then

  for try_xenstored_var in $try_xenstoreds; do
    eval "try_xenstored=\$$try_xenstored_var"
    ...

Thanks,
Ian.

-- 
Ian Jackson <ijackson at chiark.greenend.org.uk>   These opinions are my own.

If I emailed you from an address @fyvzl.net or @evade.org.uk, that is
a private address which bypasses my fierce spamfilter.