Hi, I've finally had some time to go on this review process.
Thanks for your close look at this script. Your inputs were meaningful and sounded relevant. In the end, I implemented them, as you can read in: http://anonscm.debian.org/gitweb/?p=tahoe/tahoe.git;a=blob;f=debian/tahoe-lafs.init;h=c3606db28d91f2c51880f63a56152f633ada77c1 This scripts have now evolved in something completely different from the start, which is good and point that it should probably have been written from scratch instead. More below: On Mon, Sep 23, 2013 at 11:13:17AM +0000, Zooko O'Whielacronx wrote: > * It is normally capitalized "Tahoe-LAFS" instead of "Tahoe-lafs", but > if that is a problem for some reason then I don't care. Done in git. > * 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"). Yeah, I sticked too much on the start-stop-daemon support. It wasn't taking care of the pidfile though (no --write-pidfile option passed) though. Still you're right it doesn't make sense anymore in fact. Removed in git. > * 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.) > > * 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:: > > * 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. The exit status code handling was way too inconsistent, thanks for pointing this out. I somehow noticed that in the first round of rewritings, but my limited time probably draw me to postpone-and-forget that. I believe this is corrected in git also. > * 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. > > 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. The way it was implemented was mostly due to this script being a rewrite of another. I think the "stop" action was implemented this way because it is also used when the system is told to shut down. Thinking about it, the benefits you list to have stop and start handling the AUTOSTART option the same way sounds much more relevant. The script also becomes much more robust. I believe it is now in a much more usable state and can probably be merged for the next Debian package release. bert. -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
