A friend recently contacted me with a requirement for asterisk to be protected against crashes in the same fashion as the safe_asterisk script does. Although I had not personally used the feature before, it occurred to me that exporting ASTERISK_WRAPPER="yes" would meet his requirements by triggering the usage of asterisk_run_loop(). However, it became clear that, even after defining the variable, the function in question was simply not being called. After some deliberation, I discovered that it could be fixed by changing: if [ "$(echo ${ASTERISK_WRAPPER} | tr '[:upper:]' '[:lower:]')" != "yes" ]; then ... to ... if [ "`echo ${ASTERISK_WRAPPER} | tr '[:upper:]' '[:lower:]'`" != "yes" ]; then I confirmed this by calling logger from the function, so that I could be sure that it was being started. Thus far, I've confirmed that this is an issue on two independent systems - one of which is running openrc and one of which is not. You may recall that I voiced some concern about the bashisms introduced by the most recent init.d script update through IRC. I do not know what the policy is on bashisms in init scripts nor whether there is a formal way to render them acceptable; what I do know is that this is a genuine issue that I've been able to reproduce. Even in the event that bashisms can be supported, I think that they should be avoided unless there is a very compelling reason not to. I'm attaching a patch against my current 1.4 init.d script. The original script is almost identical to the current 1.6 script so it should apply cleanly (or trivially). Changes: * Add logger statement to beginning of asterisk_run_loop() for certainty * Change $() substitution syntax to `` * Change date syntax from %Y%m%d-%h%M%s to +%Y%m%d-%k%M%S [1] * Use expr for portable arithemtic when defining $signal [2] * Double quote $USER for comparison to protect against possible pollution caused by defining ASTERISK_USER inappropriately [1] %k%M%s is very likely what the original author intended [2] Note that expr is already used elsewhere in the script
Created attachment 223425 [details, diff] Remove bashisms from init.d/asterisk
Created attachment 223427 [details, diff] Remove bashisms from init.d/asterisk (errata) Last hunk needed to call on cat within the backticks. Here's the corrected patch.
Hmm, script works for me as is, however, I suspect you're not using /bin/bash as your system shell? Hunk 1: I can confirm that $() is bash specific, even though the syntax is definitely cleaner, if this breaks non-bash shells and people actually want to use non-bash system shells then it should be changed. Seconded. The second hunk doesn't change any existing behavior, but I agree that I like the change, even if only for being able to audit the script. Suggested change seconded. Hunk 3 is again $() => ``. Change is good. Hunk 4 is good. Hunk 5 is good. Hunk 6 is good, but I'd like to suggest using different variable names instead of USER and GROUP here to prevent clashing with shell variables anyway. Hunk 7 is good, however, an alternative mechanism might be to rely purely on find, may be more efficient but will lose the ewarn status, so perhaps at least just eliminate the stat calls: for element in `find /var/{run,log}/asterisk ! -user "${USER}"`; do ewarn ... ... done This still leaves an issue where if any of the intended elements contains a space issues will occur. It's still possible to get newline chars into filenames, but much less probable, so I would suggest changing that loop to: find /var/{run,log}/asterisk ! -user "${USER}" | while read element; do ... done To eliminate at least the problem with spaces and tabs. Additionally, the test currently only tests for ownership but then fixes permissions recursively too. Do we want recursive here? It doesn't just fix the element in question - it fixes from the root up (which can be huge ... I've got >1TB setups in /var/spool/asterisk/monitor, which isn't a problem any more seeing that the scripts now leave that folder alone). Perhaps the find should be: find /var/{run,log}/asterisk ! -user ${USER} -o ! -perm -u=rw Which will find anything that isn't owned by ${USER} as well as everything that doesn't have at least rw for the user. We ideally want x on folders too but that makes life a bit more complicated (untested): ( -type d -perm -u=rwx -o -type f -perm -u=rw ) So my suggestion is to keep it simple unless required to complicate. Hunks 8 + 9 looks good again. Patch carries my approval - Tony, I suggest merging.
Kerin - please use %H instead of %k. %H uses 00 .. 23 which when running an ls just looks cleaner (and will sort better).
Please address comment #4, I will then commit.
Created attachment 234745 [details, diff] Remove bashisms from init.d/asterisk (errata 2)
Comment on attachment 234745 [details, diff] Remove bashisms from init.d/asterisk (errata 2) Replaced %k with %H
This looks ready for merging.
Tony - I thought this was merged already?
Sorry for the lack of input lately, I've been frightfully busy with work. As bash is fully supported in init scripts, the behaviour of the script should not have changed on account of using backticks but, for some bizarre reason, it did under the circumstances. I never did establish the underlying cause for this discrepancy. Whatever the case, I think it would still be reasonable to treat this contribution as a worthwhile refactoring of the existing script which also has the property of being more portable. I want to also let it be known that I am working on a pure bash init script which is quite superior in design (largely thanks to kojiro's astonishing shell scripting abilities). I'll open a separate enhancement bug for that in due course for discussion and broader testing.
+*asterisk-1.6.2.11-r1 (20 Aug 2010) + + 20 Aug 2010; <chainsaw@gentoo.org> asterisk-1.2.40.ebuild, + asterisk-1.6.2.10.ebuild, asterisk-1.6.2.11.ebuild, + +asterisk-1.6.2.11-r1.ebuild, + +files/1.6.2/asterisk-1.6.2.11-strip-noapi.patch, + +files/1.6.2/asterisk.initd2, +files/1.6.2/asterisk.logrotate, + asterisk-1.2.37.ebuild: + Tweak nm call to avoid binary deletion as requested by Alexey McSakoff in + bug #302736. Bashims removed from init script by Kerin Millar & Federico + Santulli, closes bug #309277. Stock audio prompts now split out to + separate ebuilds and logrotate support thanks to Jaco Kroon, closes bug + #328513 & #329281 respectively.
(In reply to Jaco Kroon from comment #3) > Hmm, script works for me as is, however, I suspect you're not using > /bin/bash as your system shell? > > Hunk 1: I can confirm that $() is bash specific, even though the syntax is > definitely cleaner, if this breaks non-bash shells and people actually want > to use non-bash system shells then it should be changed. Seconded. > That is wrong, both are defined in POSIX http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06_03
(In reply to Julian Ospald (hasufell) from comment #12) err yeah, this change makes no sense. if the shell sucks and doesn't have $(...), then the shell sucks and that's the shell's problem. we aim for POSIX, and we actively encourage people to not use `...` (due to nesting and it just looks better).
Comment #13 is correct, of course, and this big is probably invalid. Unfortunately, all I can remember at this juncture is that applying the patch solved the problem described in the first comment (as counter-intuitive as it may seem). However, that was a long time ago, things have moved on and I'm no longer in a position to evaluate what exactly the problem was.
(In reply to Kerin Millar from comment #14) you don't happen to recall what shell you were testing with at the time ? dash or a busybox variant ?
(In reply to SpanKY from comment #15) > (In reply to Kerin Millar from comment #14) > > you don't happen to recall what shell you were testing with at the time ? > dash or a busybox variant ? I can't be sure but I don't think there were any alternate shells installed, (In reply to SpanKY from comment #15) > (In reply to Kerin Millar from comment #14) > > you don't happen to recall what shell you were testing with at the time ? > dash or a busybox variant ? Sorry, I can't be certain. I only recall applying the modifications to the runscript and the problem being mysteriously resolved. I don't recall testing with anything other than bash. I realize that it makes no sense and that requesting to remove bashisms was an improper response to the situation. I'm far more experienced with shell programming now and would not have dealt with it now as I did then. Yet, it worked out for some reason.
Hi Kerin, The reason this came back up is because of https://bugs.gentoo.org/show_bug.cgi?id=473224 - we're trying to make sure that we won't break things for you (or your friend for that matter) by applying the init script there. If you have no objections I'm going to push Tony (Chainsaw) to get me a -r1 for 11.4.0 early next week. So *any* feedback you can provide to prevent breakage would be appreciated. Kind Regards, Jaco
