The ebuilds changed how the init.d functions when adding asterisk 1.6.26, commit: https://github.com/gentoo-mirror/gentoo/commit/44c5b5ebd5fe542c0f22ae09b1cf57b37dc6cdbd Here we start to use asterisk_wrapper, which is written in bash style with arrays (and some reg-exp stuff) I'm keen to use asterisk in a minimal environment for embedded and we had previously avoided installing bash (as it's a big space user), so this is preventing us upgrading to latest asterisk. I think I can revert to using the older init.d as a workaround, but is there appetite to migrate the asterisk_wrapper to ash shell compatible? It doesn't seem impossible, as we mainly use the arrays to construct the command run params. Thoughts?
Created attachment 859524 [details, diff] asterisk_wrapper converted to plain sh Could I propose the following change, which makes the asterisk_wrapper be bash/ash compatible.
Hi Ed, For most of these changes I'm OK with it. The one thing I don't like is the elimination of the array. In this case I *think* it's OK, but we need to be able to show that we don't need spaces in arguments, *ever*. Alternatively, we are going to have to deal with that by using ast_cmd="${ast_cmd} $(printf "%q" "${arg}")" kind of constructs. You currently also end up having a leading space in ast_cmd in all possible uses, this can be eliminated by changing all lines affecting this variable prior to line 133 to have a tailing space added, and not adding the space on this line. This is cosmetic so really a nitpick. The real proplem is ast_opts (line 86 specifically) which should probably be converted to something like: ast_opts="" for o in "$@"; do ast_opts="${ast_opts} $(printf "%q" "${o}")" done Then line 133 can be adjusted to not add the space between ast_cmd and ast_opts since that space will be in ast_opts ... nitpick as per above, ignore this one. But the change on line 86 is critical, also, for anything else that gets added into ast_cmd or ast_opts that could conceivable contain a space, now or potentially in the future.
(In reply to Jaco Kroon from comment #2) > Hi Ed, > > For most of these changes I'm OK with it. > > The one thing I don't like is the elimination of the array. Without disagreeing, this is a bash-ism, and as a general rule, the whole of gentoo appears to try and avoid bashisms and arrays? eg config.d and networking, and all the rc init files. I'm not a gentoo dev, so I mention the above as an observation rather than trying to influence anything > but we need to be able to show that we don't need spaces in > arguments, *ever*. Again, not disagreeing, although, it's worth pointing out that this is where we were on the previous version of asterisk init.d, when it was implemented inside the init.d file itself Hard to predict the future here, but it's an observable truism that people do go a long way out of their way to avoid spaces in command params in general, due to the difficulty of handling them I wonder if it's acceptable to simply document the limitations for now? Asterisk is pretty mature now. I might hope that any extra config items we choose to handle that have any hairy implications would either get punted to the asterisk config files, or handled as very special conditions in the init script. Broadly we don't want too much stuff handled in the init as it really belongs in asterisk config (especially now it's grown it's realtime config handling significantly) Would you mind giving me a specific brief on adjusting the patch so that it's acceptable for inclusion? Or perhaps it's close enough you couldn't mind adding any tweaks to make it acceptable? I'll get any changes submitted ASAP? Thanks for considering this change.
(It's indeed invalid to use bashisms in init scripts.)
(In reply to Ed Wildgoose from comment #3) > (In reply to Jaco Kroon from comment #2) > > Hi Ed, > > > > For most of these changes I'm OK with it. > > > > The one thing I don't like is the elimination of the array. > > Without disagreeing, this is a bash-ism, and as a general rule, the whole of > gentoo appears to try and avoid bashisms and arrays? eg config.d and > networking, and all the rc init files. > > I'm not a gentoo dev, so I mention the above as an observation rather than > trying to influence anything No stress, trying to help here. Agreed it's a bash-ism, these are not to be had in conf.d and init.d, which is why this was moved into a separate file since prior to this change that was violated since these bashisms was present in the init.d file. > > but we need to be able to show that we don't need spaces in > > arguments, *ever*. > > Again, not disagreeing, although, it's worth pointing out that this is where > we were on the previous version of asterisk init.d, when it was implemented > inside the init.d file itself > > Hard to predict the future here, but it's an observable truism that people > do go a long way out of their way to avoid spaces in command params in > general, due to the difficulty of handling them > > I wonder if it's acceptable to simply document the limitations for now? I'm in two minds about that - where would you put the documentation to make sure it's seen? > Asterisk is pretty mature now. I might hope that any extra config items we > choose to handle that have any hairy implications would either get punted to > the asterisk config files, or handled as very special conditions in the init > script. Broadly we don't want too much stuff handled in the init as it > really belongs in asterisk config (especially now it's grown it's realtime > config handling significantly) Fully agreed. > Would you mind giving me a specific brief on adjusting the patch so that > it's acceptable for inclusion? Or perhaps it's close enough you couldn't > mind adding any tweaks to make it acceptable? I'll get any changes submitted > ASAP? The only thing I would have liked was printf "%q" but: jkroon@plastiekpoot ~ $ /bin/busybox sh ~ $ printf "%q" "foo bar" sh: %q: invalid format ~ $ So that's not going to fly. @Sam - would appreciate your input here. In principle I'm for the patch, how is spaces generally handled in something like this? Does sh handle this in "$@" - in which case we could use "$@" and if we need to add arguments we can do set -- "$@" --extra "arguments even with spaces" then when we execute asterisk itself, we can use $ast_cmd "$@" rather perhaps?
(In reply to Sam James from comment #4) > (It's indeed invalid to use bashisms in init scripts.) This isn't in an init script, we moved this into a separate file specifically avoid that issue.
(In reply to Jaco Kroon from comment #5) > @Sam - would appreciate your input here. In principle I'm for the patch, > how is spaces generally handled in something like this? Does sh handle this > in "$@" - in which case we could use "$@" and if we need to add arguments we > can do set -- "$@" --extra "arguments even with spaces" then when we execute > asterisk itself, we can use $ast_cmd "$@" rather perhaps? Yes, sh does handle this. The attached patch is not sufficient to render the script posix-compliant. I did some work on this but quietly abandoned it upon the decision being made to target bash. Assuming there is an interest in using sh once again, I could dig it out and submit it. The principal challenge would be in determining what to do with the ulimit commands, because there is no use of ulimit that is portable beyond ulimit -f. At the time, my plan was to propose that https://github.com/addaleax/rlimitr be incorporated and made a runtime dependency. Such a utility would also be useful for addressing the likes of bug 739274.
Created attachment 861435 [details, diff] jkroon proposed diff This is the diff for what I'm suggesting based on the discussion here. Going to upload the full file as well in a moment.
Created attachment 861436 [details] asterisk_wrapper
Already fixed prlimit not getting passed with --pid=$$ ... without this it seems prlimit doesn't update the current shell, which negates the point of this. This script should be safe, and basic testing checks out OK.
Regarding the use of prlimit(1), good idea. I've only very briefly skimmed the commit but I would point out that the way in which SHELL is being used is wrong. That is because the sole purpose of SHELL is to define the current user's login shell, according to their passwd entry. It does not define the currently executing shell, nor does it define the currently effective sh(1) implementation. The appropriate way to rectify this issue is to jettison the definition of $echo_e and disavow the use of echo altogether. As the POSIX documentation states, "if the first operand is -n, or if any of the operands contain a <backslash> character, the results are implementation-defined." It goes on to say that "it is not possible to use echo portably across all POSIX systems unless both -n (as the first argument) and escape sequences are omitted." In other words, echo is a bad utility. Instead, consider the following: printf '%s\r\n' "Subject: Asterisk crashed" "$MSG" Note, however, that %s will not expand any backslash-character sequences that are present in MSG. If you feel that it is imperative to continue to incorporate such sequences directly within the value of MSG (I wouldn't) then you may use the %b specifier where appropriate. printf '%s\r\n%b\r\n' "Subject: Asterisk crashed" "$MSG" That aside, any 'conventional' use of echo should be replaced with printf '%s\n'.
Or, if you prefer ... printf 'Subject: Asterisk crashed\r\n%b\r\n' "$MSG"
The bug has been closed via the following commit(s): https://gitweb.gentoo.org/repo/gentoo.git/commit/?id=0141572cd12dcf374a9c2a3d982f0afb895a43fa commit 0141572cd12dcf374a9c2a3d982f0afb895a43fa Author: Jaco Kroon <jaco@uls.co.za> AuthorDate: 2023-05-10 21:36:13 +0000 Commit: Sam James <sam@gentoo.org> CommitDate: 2023-05-11 01:28:39 +0000 net-misc/asterisk: add 20.2.1 Closes: https://bugs.gentoo.org/903912 Closes: https://bugs.gentoo.org/903913 Closes: https://bugs.gentoo.org/903775 Signed-off-by: Jaco Kroon <jaco@uls.co.za> Signed-off-by: Sam James <sam@gentoo.org> net-misc/asterisk/Manifest | 1 + net-misc/asterisk/asterisk-20.2.1.ebuild | 373 +++++++++++++++++++++++++++++++ 2 files changed, 374 insertions(+) Additionally, it has been referenced in the following commit(s): https://gitweb.gentoo.org/repo/gentoo.git/commit/?id=2cc5bb67b6b0a2b5b8351a233508ad83aafc62a2 commit 2cc5bb67b6b0a2b5b8351a233508ad83aafc62a2 Author: Jaco Kroon <jaco@uls.co.za> AuthorDate: 2023-05-10 21:32:07 +0000 Commit: Sam James <sam@gentoo.org> CommitDate: 2023-05-11 01:28:39 +0000 net-misc/asterisk: add 18.17.1 Bug: https://bugs.gentoo.org/903912 Bug: https://bugs.gentoo.org/903913 Bug: https://bugs.gentoo.org/903775 Signed-off-by: Jaco Kroon <jaco@uls.co.za> Signed-off-by: Sam James <sam@gentoo.org> net-misc/asterisk/Manifest | 1 + net-misc/asterisk/asterisk-18.17.1.ebuild | 377 +++++++++++++++++++++ ...ix-test-code-to-match-gethostbyname_r-pro.patch | 71 ++++ .../asterisk/files/asterisk_wrapper-18.17.1-20.2.1 | 227 +++++++++++++ net-misc/asterisk/files/initd-18.17.1-20.2.1 | 311 +++++++++++++++++ 5 files changed, 987 insertions(+)
