Ian Jackson
2019-Feb-12 17:21 UTC
[Pkg-xen-devel] [PATCH 08/13] xen init script: rewrite xenstored start logic
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/$XENSTOREDIMO 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 1This 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.
Hans van Kranenburg
2019-Feb-12 17:59 UTC
[Pkg-xen-devel] [PATCH 08/13] xen init script: rewrite xenstored start logic
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
Ian Jackson
2019-Feb-12 18:14 UTC
[Pkg-xen-devel] [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.