Currently, the SABnzbd init script doesn't check if the API is enabled before trying to attempt a web-based shutdown. This is less than ideal. The procedure should be re-worked to check this first then gracefully fall back to the normal SIGTERM method used by most start-stop-daemon scripts.
Created attachment 365582 [details] sabnzbd init script - improved shutdown routine. Improved shutdown routine and also removed spurious tab characters.
Created attachment 365584 [details, diff] sabnzbd init script - improved shutdown routine. (patch against 0.7.16 ebuild)
Created attachment 365586 [details] sabnzbd init script - improved shutdown routine. Removed unnecessary status message.
Created attachment 365588 [details, diff] sabnzbd init script - improved shutdown routine. (patch against 0.7.16 ebuild) Removed unnecessary status message.
Comment on attachment 365586 [details] sabnzbd init script - improved shutdown routine. --- files/sabnzbd.initd 2013-10-20 16:21:18.028447574 +0200 +++ - 2013-12-18 16:29:19.814976658 +0100 @@ -82,14 +82,18 @@ local pidfile=$(get_pidfile) local rc=1 - ebegin "Stopping SABnzbd @ ${addr}" + # This can only work if we have enabled the API + if [ -n "${api_key}" ]; then - # SABnzbd will return "ok" if shutdown is successful - rc=$(/usr/bin/curl -k -s "${addr}/sabnzbd/api?mode=shutdown&apikey=${api_key}") - if [ "${rc}" == "ok" ]; then - rc=0 - else - einfo "Falling back to SIGTERM, this may not work if you restarted via the web interface" + einfo "Attempting web-based shutdown @ ${addr}" + + # SABnzbd will return "ok" if shutdown is successful + rc=$(/usr/bin/curl -k -s "${addr}/sabnzbd/api?mode=shutdown&apikey=${api_key}") + [[ "${rc}" == "ok" ]] && rc=0 + fi + + if [ "${rc}" -eq 1 ]; then + einfo "Shutting down via SIGTERM" start-stop-daemon \ --stop \ --pidfile ${pidfile} \
Created attachment 365628 [details, diff] sabnzbd init script - improved shutdown routine. (patch against 0.7.16 ebuild) Also need to check that "disable_api_key" is not set when we have an api key present as this will still not allow us to perform an API shutdown call.
I'm going to go through this script and ensure I'm being consistent with my coding style throughout :) I think there are a few improvements I can make.
I should get this finished soon - little bit behind with it being Christmas and all :)
Created attachment 366486 [details, diff] Diff against revision 1.7 of initd script. + Fixed indenting to more widely used 2-space. + Removed spurious tab characters. + Re-worked shutdown proc to be more robust. + Proper use of quoting in line with BASH good-practices.
Created attachment 366488 [details, diff] Diff against revision 1.7 of initd script. + Fixed missed out variable.
Created attachment 366498 [details, diff] Diff against revision 1.7 of initd script. + Removed unecessary variables.
Created attachment 366500 [details, diff] Diff against revision 1.7 of initd script. + Fixed "any-address" comparison.
Created attachment 366504 [details, diff] Diff against revision 1.7 of initd script. + Further tidy-ups.
(In reply to eponymous from comment #13) > Created attachment 366504 [details, diff] [details, diff] > Diff against revision 1.7 of initd script. > > + Further tidy-ups. All set for me to review this now or do you have more pending changes?
(In reply to Justin Bronder from comment #14) > (In reply to eponymous from comment #13) > > Created attachment 366504 [details, diff] [details, diff] [details, diff] > > Diff against revision 1.7 of initd script. > > > > + Further tidy-ups. > > All set for me to review this now or do you have more pending changes? Hehe all done :) Yep if you could please then I can get any review actions done asap. Cheers!
Alright. It seems the meat of this diff is just to check for the api_key and to make sure it wasn't disabled. Is there anything else that is functionally changed? For the other changes: * Spacing, if changed at all should be to tabs to be consistent with Gentoo style guides. * I'm not sure I see the point of changing "if/else" blocks to use "test && statement". I have no real preference, but changing all of this just seems like churn that could introduce bugs. * Similar to above, the same goes for changing from using local variables to save the output of function calls against using them inline. In start_pre(), you're actually uselessly calling get_pidfile over and over for no gain. I'm happy to put the other changes in if you have a strong preference but let's fix the actual bug in the init script first so the changelog is clean.
>> Alright. It seems the meat of this diff is just to check for the api_key and to make sure it wasn't disabled. Is there anything else that is functionally changed? Yes that was the main reason for this patch. Everything else is just stylistic changes to make things consistent. >> * Spacing, if changed at all should be to tabs to be consistent with Gentoo style guides. Sorry about that, Vim hiccup. I changed my default tab spacing to be two characters rather than four - not sure why. Fixed now. >> * I'm not sure I see the point of changing "if/else" blocks to use "test && statement". I have no real preference, but changing all of this just seems like churn that could introduce bugs. This was simply to avoid unecessarily long bash if, then, else, fi blocks - they're ugly for small tests & sets. >> * Similar to above, the same goes for changing from using local variables to save the output of function calls against using them inline. In start_pre(), you're actually uselessly calling get_pidfile over and over for no gain. Yep that was indeed silly. Good spot - I've fixed this now.
Created attachment 378530 [details, diff] Diff against revision 1.7 of initd script - reworked.
19 Jun 2014; Justin Bronder <jsbronder@gentoo.org> files/sabnzbd.initd: Check that api is not disabled before attempting to use it for shutdown. Thanks to eponymous, #494640
I've just noticed that we seem to have an issue with the init script you've added to the repository in that it doesn't match the diff attached to this bug. For example, take this line: - --user ${SABNZBD_USER} \ - --group ${SABNZBD_GROUP} \ + --user "${SABNZBD_USER}" \ + --group "${SABNZBD_GROUP}" \ But in the initd file we have: 47 --user ${SABNZBD_USER} \ 48 --group ${SABNZBD_GROUP} \ Are you sure you've applied the diff correctly?
(In reply to eponymous from comment #20) > I've just noticed that we seem to have an issue with the init script you've > added to the repository in that it doesn't match the diff attached to this > bug. > > For example, take this line: > > - --user ${SABNZBD_USER} \ > - --group ${SABNZBD_GROUP} \ > + --user "${SABNZBD_USER}" \ > + --group "${SABNZBD_GROUP}" \ > > But in the initd file we have: > > 47 --user ${SABNZBD_USER} \ > 48 --group ${SABNZBD_GROUP} \ > > Are you sure you've applied the diff correctly? I handpicked the relevant parts that were not stylistic. You can see the diff here. http://sources.gentoo.org/cgi-bin/viewvc.cgi/gentoo-x86/net-nntp/sabnzbd/files/sabnzbd.initd?r1=1.7&r2=1.8 However, for the lines you picked, that should have been applied. Are you sure you have the latest ebuild?
(In reply to Justin Bronder from comment #21) > (In reply to eponymous from comment #20) > > However, for the lines you picked, that should have been applied. Are you > sure you have the latest ebuild? Whoops, I take that back. Quotes were not added there. Are you using users/groups with spaces?
So now I'm confused. I agree that *some* of my changes were stylistic but we should be consistent throughout the file. One recommended approach to variable use is to surround them with quotes and braces which I've adopted: "${VAR_NAME}" and we should either have that convention throughout the entire file or not, rather than a mix of two. I don't mind really. I've just checked: /usr/portage/net-nntp/sabnzbd/files/sabnzbd.initd again after an emerge --sync and the inconsistency still exists.
(In reply to eponymous from comment #23) > So now I'm confused. I agree that *some* of my changes were stylistic but we > should be consistent throughout the file. > > One recommended approach to variable use is to surround them with quotes and > braces which I've adopted: > "${VAR_NAME}" and we should either have that convention throughout the > entire file or not, rather than a mix of two. I don't mind really. > > I've just checked: /usr/portage/net-nntp/sabnzbd/files/sabnzbd.initd again > after an emerge --sync and the inconsistency still exists. That's a good rule for paths, but I don't think it makes much sense for things that cannot reasonably have spaces in them. Bash also doesn't need quoting when performing assignment [1]. My personal preference is to keep diffs as closely tied to a specific functional change as possible. This makes is far easier to debug months later when I've forgotten why changes were made in the first place. 1. $ b=$(echo some thing) $ a=$b $ echo $a some thing
> That's a good rule for paths, but I don't think it makes much sense for > things that cannot reasonably have spaces in them. Bash also doesn't need > quoting when performing assignment [1]. There are other instances where it is useful such as if you want to delimit where a variable name ends in a joined string. > My personal preference is to keep diffs as closely tied to a specific > functional change as possible. This makes is far easier to debug months > later when I've forgotten why changes were made in the first place. Yes that's reasonable. I'd say we should probably just leave it as is then, since in hindsight, it's probably not worth re-opening the bug and committing a new init script for stylistic changes.
