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

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.

On 2/12/19 7:14 PM, Ian Jackson wrote:
> 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
Ok.
> 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).
Yes, I'm no start-stop-daemon expert, but this should be well tested in
combination with the already-running check.

Having the init script wrongly start another xenstored just makes
everything hang withouta good explanation to the user.

K

On 2/12/19 7:21 PM, Hans van Kranenburg wrote:
> On 2/12/19 7:14 PM, Ian Jackson wrote:
>> 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
So, if I have

  /home/knorrie/xenstore-mouton

and start that, and then change it to

  /home/knorrie/xenstore-kebab

...then do /etc/init.d/xen restart, then start-stop will not see a
matching process because there's no way it can know about this
xenstore-mouton binary which is still running. I end up with stopped
xenconsoled, and then xenstore-kebab will be started, which fails and
the whole thing aborts.

Option 1: simplify check to just do -f pidfile existence, like upstream
launch-xenstore does.
Option 2: do something with the `xenstore-read -s /` check from the
timeout loop. If it connects and reads, it's apparently running, boom, done.

Or a combination... or...

Whraagh, actually, instead of continuing with this... Maybe we should
just call launch-xenstore from the init script, and butcher it a bit
with a patch in the packaging...

* Change /etc/default/xencommons to /etc/default/xen
* Change the echo line into lsb_blah.
* Change the startup command into start-stop-daemon.
* Maybe do something with the `xenstore-read -s /` check? Why is this
used in Debian in the timeout loop instead? Is it better than -f pidfile?
* We already remove the section about XENSTORETYPE in /etc/default/xen,
so the domain part in it is just inactive code.

Then we have the script with build-replaced @@ in it and are closer to
looking at changes which we might try upstreaming later (converting to
lsb_etc for example.) And, we're closer to including systemd stuff,
since that simply also reuses the launch-xenstore. (!)

Also, apparently init-dom0 is not done inside launch-xenstore, but in
xencommons:

        @XEN_SCRIPT_DIR@/launch-xenstore || exit 1

        echo Setting domain 0 name, domid and JSON config...
        ${LIBEXEC_BIN}/xen-init-dom0

So, even the patch to move that into launch-xenstore because it wil only
print "Bweuuh, I have nothing to do" in all other cases can go
upstream..

What do you think?
>> 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).
> 
> Yes, I'm no start-stop-daemon expert, but this should be well tested in
> combination with the already-running check.
> 
> Having the init script wrongly start another xenstored just makes
> everything hang withouta good explanation to the user.
K