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

[Hans van Kranenburg]

On 2/12/19 6:21 PM, Ian Jackson wrote:
> 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
> 
> ?

So the question we should answer first is: What do we allow the user to
set as value? /bin/bash? And how should the init script deal with that?

I'd rather even restrict it more, only allow exact words 'xenstored' and
'oxenstored'.

Or do you want to be able to use this to run your own xenstored
replacement implementation from /usr/local/ian/ixenstored ?

>> +	# 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.

Using --start --pidfile --exec --test apparently both checks for pidfile
and also for an actual matching running process. This construct was used
here already, I just kept it. I see why a small comment that this
returning 1 explicitely means that "Success! We found a matching process!".

Maybe simplify and just return 1 from this function if it's not good,
and return 0 on success?

The 0|1) which are in several places further on are also confusing me:

        xenstored_start
        case "$?" in
                0|1) ;;
                *) log_end_msg 1; exit ;;
        esac

I mean, why torture the reader of the code with counter intuitive 1
values that mean success...

>> +	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"
>     ...

Yes, that way the two start-stop-daemon can be one instead, nice!

And do you agree that exploding when an explicit choice has been made by
the user + that binary can't be started is the right thing?

If I would accidentally set 'xensrtored' as value, because I explicitely
want to run xenstored and not oxenstored, and the typo is because I'm in
a hurry because a gigantic security hole in oxenstored was found, I
don't want the init script to silently still start oxenstored, because
-x /[...]/xensrtored fails.

K