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

[Ian Jackson]

Hans van Kranenburg writes ("Re: [PATCH 08/13] xen init script: rewrite xenstored start logic"):
> 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?

If they specify an absolute path, it should be used.  Eg,
  /home/alice/Xen/xen/tools/xenstore/xenstore
If the user specifies /bin/bash they get to keep all the resulting
pieces.

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

Right.  Or a wrapper script or something (although s-s-d will
mishandle that).

> > 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?

That would be more usual.

Normally an init script which is asked to start a daemon and declines
to do so because it's already running will treat that as a failure.

But we don't ever stop xenstored so I think in this case we should
probably treat it already being there as a success for `start' ?

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

I agree.  Madness.

> >   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?

Yes.

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

Indeed.

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.