Okay, here I'm reviewing
http://anonscm.debian.org/gitweb/?p=tahoe/tahoe.git;a=blob;f=debian/tahoe-lafs.init;h=dda97b498f33330f47b3d284fd4228e8e0a1cd2b;hb=refs/heads/feature/sysvinit
.
Thank you for working on this patch! You seem to have addressed all of
the issues that I raised in my previous review.
* It is normally capitalized "Tahoe-LAFS" instead of "Tahoe-lafs", but
if that is a problem for some reason then I don't care.
* The "tahoe" command and the start-stop-daemon are both going
simultaneously write the PID into the file named "twistd.pid", if I
understand correctly. That seems like it might cause trouble, and also
seems unnecessary. Suggest removing the configuration which tells
start-stop-daemon to do that part ("--pidfile").
* In fact, what is start-stop-daemon needed for, if we remove the
--pidfile? Maybe replace::
47 start-stop-daemon --start --quiet --oknodo \
48 --pidfile "$CONFIG_DIR/${node_name}/twistd.pid" \
49 --exec $DAEMON --chuid "$node_uid" -- \
50 start $DAEMONARGS
"$CONFIG_DIR/${node_name}" > /dev/null || STATUS=1
with::
47 su -s "/bin/sh" -c "$DAEMON start
$CONFIG_DIR/${node_name}" "$node_uid" > /dev/null
48 STATUS=$?
This would also mean that STATUS gets updated to reflect the exit
code from "tahoe", instead of being set to 1 iff start-stop-daemon
exited with non-zero. (More about STATUS, below.)
* On Line 52, when it refuses to start a node owned by root, should
probably set STATUS (to 1). (More on STATUS below.)
* Line 71 checks if "$AUTOSTART" is the zero-length string, and if so
exits, but then line 75 checks if "$AUTOSTART" is the length-zero
string, which it never will be. I suggest to change line 75 from::
if test -z "$AUTOSTART" -o "$AUTOSTART" = "all" ; then
to::
if test "$AUTOSTART" = "all" ; then"
* "AUtOSTART" → "AUTOSTART"
* Lines 83 and 95 test whether a directory exists before running
"tahoe start" on it, and if it doesn't exist it, they print out an
error message (" No such node configured: $name"). However, if this
code were removed from the init script, and instead the init script
tried to run "tahoe start" on the directory without checking, then
tahoe would print out a similar error message, like this::
$ tahoe start blahblahblah
STARTING '/home/zooko/blahblahblah'
'/home/zooko/blahblahblah' does not look like a directory at all
I would recommend omitting the test within the init script for the
following reasons:
1. The tests performed by tahoe can be more specific and informative
than the one performed by the init script, for example, if the
directory exists but is empty::
$ mkdir blahblahblah
$ tahoe start blahblahblah
STARTING '/home/zooko/blahblahblah'
'/home/zooko/blahblahblah' does not look like a node directory (no
.tac file)
2. If all such error messages come from the same source, they will
be easier for users to understand.
3. The more code there is, the more places a bug-hunter has to
search when looking for a bug (even if the bug turns out not to be in
that code), so if we can optimize-out code we should! This is
especially true of code that is potentially redundant. For example, if
a bug-hunter or security auditor is investigating something involving
erroring-out when a directory doesn't exist, and they see the code for
that in tahoe but not in the init script, or vice versa, then they
might mistakenly think that they finished examining that
functionality, and not realize that there is a semi-redundant
implementation of that functionality elsewhere, that could be
interacting with the bug.
* This same reasoning applies to the detection of whether the
twistd.pid file is missing, on line 117. If that test is omitted, then
instead of the init script saying "No such node running: $name", tahoe
will say::
$ tahoe stop gateway/
STOPPING '/home/zooko/blahblahblah'
No such process
* What should the init script do with STATUS when there are multiple
directories and some of them fail but others don't? Currently it
overwrites STATUS with the result of each directory, so at the end the
STATUS variable will reflect the result of the final directory. I
think it would be better for STATUS to be 0 only when *all* of the
directories that it tried succeeded. If there are any errors, then
STATUS should be non-zero at the end.
* Shouldn't handling of "AUTOSTART" during "/etc/init.d/tahoe-lafs
start" be done likewise during "/etc/init.d/tahoe-lafs stop"?
Currently if AUTOSTART is "none" or the zero-length string then
running "/etc/init.d/tahoe-lafs start" will result in an error message
saying 'Autostart disabled' (line 71), but running
"/etc/init.d/tahoe-lafs stop" will result in stopping all of the
running nodes associated with the directories. I suggest to copy the
code from line 71 into the "stop)" and "restart)" blocks so that the
behavior will be more consistent, i.e. as determined by the arguments
on the command-line, the value of the AUTOSTART variable, and which
node directories exist.
* What should happen if the user asks the init script to stop all of
the tahoe nodes, and some of them are running but others aren't?
Possible answers:
a. The ones that were running are stopped, the others are ignored,
final STATUS is 0 (assuming all the ones that were running succeeded
at stopping), or
b. The ones that were running are stopped, and a warning message is
printed for the ones that weren't running, and STATUS is still 0
(assuming all the ones that were running succeeded at stopping), or
c. The ones that were running are stopped, and a warning message is
printed for the ones that weren't running, and STATUS is non-zero to
indicate that they couldn't be stopped (because they weren't running).
Probably different ones of these will be preferred by different
users at different times. However, I would suggest c), because it
gives more potentially-useful information to the user, and also
because we can then optimize some more code out of the init script,
delegating more of the work to tahoe, and making the init script's
"/etc/init.d/tahoe-lafs start" behavior more similar to its
"/etc/init.d/tahoe-lafs stop" behavior. Specifically, I suggest the
init script replaces::
131 for pidfile in `find $CONFIG_DIR -name twistd.pid` ; do
132 name=`echo "$pidfile" | cut -c21- | sed -e
's/\/twistd.pid//'`
133 _tahoe restart "$name"
134 done
with::
131 for name in `cd $CONFIG_DIR; find ./ -mindepth 1
-maxdepth 1 -type d | cut -c3-`; do
132 _tahoe stop "$name"
133 done
or, if the suggestion above is accepted, to make the choice of which
nodes are affected by "stop" be the same as which nodes are affected
by "start", then it would be::
131 if test -z "$AUTOSTART" -o "$AUTOSTART" = "all" ; then
132 # all nodes shall be stopped automatically
133 for name in `cd $CONFIG_DIR; find ./ -mindepth 1
-maxdepth 1 -type d | cut -c3-`; do
134 _tahoe stop "$name"
135 done
136 else
137 # stop only nodes specified in $AUTOSTART
138 for name in "$AUTOSTART" ; do
139 _tahoe stop "$name"
140 done
141 fi
This would have the following behavior:
1. "/etc/init.d/tahoe-lafs stop" (when in AUTOSTART mode) would
print out warning messages about any nodes that were not running (and
therefore couldn't be stopped).
2. It would end with STATUS=0 only if all of the nodes that it tried
to stop were running and they all successfully stopped.
3. If a future version of Tahoe-LAFS stops using the twistd.pid to
indicate whether a node is running, this will not require any changes
to the init script.
--
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
