-
Notifications
You must be signed in to change notification settings - Fork 254
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
asterisk-16: new package #411
Conversation
Oh damn... just noticed this now :( |
Good contributions from both of you! We should find out what is causing the segfaults. @dhewg are you experiencing the mentioned segfaults in your version of ast16 package? |
I did, and I fixed the same spot, just a bit differently. commit As does |
Thanks for chiming in. I was bummed out a bit by the astmm.h thing. I thought that maybe if we fix that differently then the segfault will disappear. So I asked for help again on the must list here. It was suggested to simply include sched.h before the redefinitions. So I changed the astmm.h patch to this:
That actually works fine, compile failures are gone. But I was too optimistic thinking that this might also fix the segfault :-) Kind regards, |
The responses on the musl list reflect my own findings: code of poor quality on asterisk's side. |
On Tue, Mar 05, 2019 at 10:44:41PM -0800, dhewg wrote:
The responses on the musl list reflect my own findings: code of poor
quality on asterisk's side. This whole ASTMM_LIBC thing is for
debugging anyway. I recommend just enforcing ASTMM_IGNORE and be done
with it.
Hello,
I agree in so far as the patch I posted is lacking. I think the correct
place the address this would be in the source files that include
"asterisk.h" _and_ <sched.h>. But this might be a bit more involved, so
for the time being I think ASTMM_IGNORE is the way to go.
Regarding the segfaults. I'm still trying to get to the bottom of this.
It's fun, but time-consuming. Yesterday I made a little progress. I
think I'll again post to the musl list later today, with more details,
to ask for their insight.
Kind regards,
Seb
|
There are of course better ways to fix this. But I'm not going to sign any CLA to submit a proper fix upstream. So my answer was meant as a way to get this working on openwrt with a simple and future proof patch. If you're up to upstream a proper fix - I'm all for it!
That |
Hello again, Don't have something off-hand for the astmm.h header. But I'm not really worried about that either as it all worked fine in the past without the redefinitions. But the module loader issue does put the fear in me. The loader is complex and the fact that on musl dlopen will only run the constructor once doesn't fit in with the loader's inner workings. I agree that the workaround (preventing the segfault) seems to work. But there's a good chance that something isn't working that we're not aware of. The only other project that defaults to musl is Alpine Linux afaik. And they didn't upgrade to Asterisk 16 yet, they still run 15. I'm venturing a guess here that they've seen the breakage, too, and are being cautious for now. I'd rather wait till a proper workaround is available (and honestly/realistically it'll likely not be from me). Just my 2 cents. :) Kind regards, |
That might well be, in fact I now get these warnings at startup:
I didn't investigate, but to me it looks like their loader relies on a behavior that only holds on glibc. Those modules are loaded (
Oh, I didn't know about alpine and musl, did you have contact with the asterisk packagers before? I can only reiterate, but I do have asterisk16 running with these patches on openwrt for some weeks. And that's my business line, so I guess +100 calls now. I don't mind waiting to merge asterisk16 support, but we can replace our patches at any time once there's a better/proper fix. But i guess that's going to be a rewrite of parts of their loader due to their presumptions. |
Hi all, I updated the astmm.h patch a bit (mostly comments). And for the loader I started on a workaround. But something is not right with it. If anybody wants to test or has any comments, please do so. Best regards, |
I reworked the loader patch. Now both Adding @fabled from Alpine and @jacmet, @bkuhls and @yann-morin-1998 from Buildroot in the hopes of getting some more eyes on the loader/astmm patches. Both Alpine and Buildroot use musl, too, so I think there might be a common interest here :-) I'm not a programmer, so I probably made some mistakes. If anybody could test/review this and provide feedback that would be cool. Kind regards, |
Hi all, Updated the loader patch. For one I removed the OpenWrt Makefile part to make it easier to test if you don't use OpenWrt. The other changes:
|
Oh, and I changed the name of the 2nd argument of both ast_manual_module_register() and ast_manual_module_unregister() from "name" to "resource" to make the point that this is not the name ("super_mod") but the file ("super_mod.so"). |
Quick update, loader patch is being reviewed upstream here Corey Farrell was nice enough to provide lots of feedback; changes were made. Will update the PR once final. Kind regards, |
c27f222
to
1490309
Compare
Hello Jiri, @dhewg et al, I have updated the loader patch with the one that is now in upstream's gerrit. It was reviewed by Corey from Digium who also cherry-picked it for 16. It has not been committed yet, but that may take some more time. I'd like to ask that you give it a test run. I have run-tested this and it looks good to me. If this gets merged I can open another pull request to add the out-of-tree modules. As a side note I would suggests to think about removing both Asterisk 13 and 15. My arguments would be the following:
Looking forward to hear from you :) With kind regards, |
Nice work! Did you sign their contributor stuff? I updated #412 with just the additions and build with that on top of this (since I depend on those modules). Everything built just fine and seems to work with just a quick test. I'll test it a few days to make sure. I got this message though, I think that's new?
|
Hello @dhewg I signed it some time ago for some other stuff, so no extra effort there :) cdr_syslog only loads when you provide proper config for it (not installed by default), so that is normal I take it. Kind regards, Edit: I get this also on x86_64 with glibc (so without the loader patch). |
It's been working fine for me, no issues at all. I'd say this can go in ;) |
Thank you for testing!
|
I acknowledge that asterisk works fine. |
Hi Jiri,
The loader patch got merged upstream today :-)
Regards,
Seb
|
Nice work! I am looking to upgrade Alpine Asterisk soon. Seems that a simpler fix for the astmm issue might be: --- a/include/asterisk/compat.h
+++ b/include/asterisk/compat.h
@@ -30,6 +30,7 @@
#include <inttypes.h>
#include <limits.h>
#include <unistd.h>
+#include <pthread.h>
#ifdef HAVE_STDDEF_H
#include <stddef.h>
See also: alpinelinux/aports@a752e63 |
Hi Timo, That seems to work fine. I understand that this way sched.h is included before astmm.h and that's why the compile does not fail. Could you tell me though why you chose to include pthread.h instead of sched.h? Kind regards, |
The header inclusion chain seems to be |
Thank you Timo!
|
Hi Jiri, I updated the commit with Timo's astmm fix. Compile-tested on ath79 (musl) and arc (uclibc), run-tested on ath79. Kind regards, |
Hi Sebastian, thank you for your hard work. I have made several tests and tried Asterisk 16 in production and it seems to be stable. Would you be so kind to squash those commits into one? Thank you. Kind Regards, |
Hi Jiri, Commits squashed. Kind regards, |
Thanks for your work, is it able to adapt and cover #369 |
Not within this PR. This is completely different task. It does not belong to this PR. |
Hi Jiri, will do tonight. Regards, Seb
|
Initial commit of Asterisk 16. Cleans up Makefile; the version number now only occurs once in it. Upstream removed the following modules: - format_jpeg - res_pjsip_registrar_expire (functionality was moved into res_pjsip_registrar.) pjsip has a new dependency, res-http-websocket. Notes: - replaced res_ninit patch Replaced patch with the one from Alpine. It's a bit more flexible and allows usage of res_ninit where available (when building against glibc). - fixed musl compiles astmm.h now always gets included by asterisk.h, redefining allocators. This causes breakage on musl: ccache_cc -o chan_pjsip.o -c chan_pjsip.c -MD -MT chan_pjsip.o -MF .chan_pjsip.o.d -MP -pthread -I/home/sk/tmp/openwrt/build_dir/target-mips_24kc_musl/asterisk-16.2.1/include -Os -pipe -mno-branch-likely -mips32r2 -mtune=24kc -fno-caller-saves -fno-plt -fhonour-copts -Wno-error=unused-but-set-variable -Wno-error=unused-result -msoft-float -mips16 -minterlink-mips16 -iremap/home/sk/tmp/openwrt/build_dir/target-mips_24kc_musl/asterisk-16.2.1:asterisk-16.2.1 -Wformat -Werror=format-security -fstack-protector -D_FORTIFY_SOURCE=1 -Wl,-z,now -Wl,-z,relro -I/home/sk/tmp/openwrt/staging_dir/target-mips_24kc_musl/usr/lib/libiconv-stub/include -I/home/sk/tmp/openwrt/staging_dir/target-mips_24kc_musl/usr/lib/libintl-stub/include -I/home/sk/tmp/openwrt/staging_dir/target-mips_24kc_musl/usr/include -I/home/sk/tmp/openwrt/staging_dir/target-mips_24kc_musl/include -I/home/sk/tmp/openwrt/staging_dir/toolchain-mips_24kc_gcc-7.4.0_musl/usr/include -I/home/sk/tmp/openwrt/staging_dir/toolchain-mips_24kc_gcc-7.4.0_musl/include/fortify -I/home/sk/tmp/openwrt/staging_dir/toolchain-mips_24kc_gcc-7.4.0_musl/include -I/home/sk/tmp/openwrt/staging_dir/target-mips_24kc_musl/usr/lib/libiconv-stub/include -I/home/sk/tmp/openwrt/staging_dir/target-mips_24kc_musl/usr/lib/libintl-stub/include -I/home/sk/tmp/openwrt/staging_dir/target-mips_24kc_musl/usr/include/libxml2 -Wall -Wstrict-prototypes -Wmissing-prototypes -Wmissing-declarations -fPIC -DAST_MODULE=\"chan_pjsip\" -DAST_MODULE_SELF_SYM=__internal_chan_pjsip_self -DPJ_AUTOCONF=1 -DPJ_IS_BIG_ENDIAN=1 -DPJ_IS_LITTLE_ENDIAN=0 -fPIC -I/home/sk/tmp/openwrt/staging_dir/target-mips_24kc_musl/usr/include In file included from /home/sk/tmp/openwrt/build_dir/target-mips_24kc_musl/asterisk-16.2.1/include/asterisk.h:23:0, from chan_pjsip.c:35: /home/sk/tmp/openwrt/build_dir/target-mips_24kc_musl/asterisk-16.2.1/include/asterisk/astmm.h:158:35: error: expected '=', ',', ';', 'asm' or '__attribute__' before '->' token Do_not_use_calloc__use_ast_calloc->fail(a, b) ^ /home/sk/tmp/openwrt/build_dir/target-mips_24kc_musl/asterisk-16.2.1/include/asterisk/astmm.h:162:77: error: expected '=', ',', ';', 'asm' or '__attribute__' before '->' token Do_not_use_free__use_ast_free_or_ast_std_free_for_remotely_allocated_memory->fail(a) ^ make[4]: *** [/home/sk/tmp/openwrt/build_dir/target-mips_24kc_musl/asterisk-16.2.1/Makefile.rules:153: chan_pjsip.o] Error 1 The problem is that with _GNU_SOURCE defined musl also declares calloc in <sched.h> - and when asterisk's source includes <sched.h> _after_ "asterisk/astmm.h" the definition clashes with the macro. Timo Teräs from Alpine Linux fixed this by including <pthread.h> in "asterisk/compat.h". He chose to include <pthread.h> instead of <sched.h> because the original header inclusion chain seems to be "asterisk/astobj2.h" -> "asterisk/lock.h" -> <pthread.h> -> <sched.h>. It seems Asterisk practically never includes <sched.h> directly. - added loader workaround for musl When the modules are loaded, asterisk segfaults on musl. Asterisk Dynamic Loader Starting: [Mar 2 22:30:05] NOTICE[20712]: loader.c:2230 load_modules: 91 modules will be loaded. Segmentation fault [48817.544248] do_page_fault(): sending SIGSEGV to asterisk for invalid read access from 00000000 [48817.544258] epc = 77f6b764 in libc.so[77ef8000+94000] [48817.544285] ra = 0048d579 in asterisk[400000+160000] The real problem is that the loader expects dlopen to always run the constructor, which doesn't happen with musl, because its dlopen is permanent. This commit adds a new configure switch '--enable-permanent-dlopen'. When enabled, the loader will manually call 'ast_module_register(...)' and 'ast_module_unregister(...)' when needed. - allow eventfd detection Asterisk 16 wants to use eventfd, but it doesn't allow the detection during cross-compiling. This results in runtime warnings, for instance when shutting down: [Mar 2 22:37:41] WARNING[21593]: alertpipe.c:112 ast_alertpipe_read: read() failed: Bad file descriptor [Mar 2 22:37:41] WARNING[21593]: alertpipe.c:112 ast_alertpipe_read: read() failed: Bad file descriptor [Mar 2 22:37:41] WARNING[21593]: alertpipe.c:112 ast_alertpipe_read: read() failed: Bad file descriptor Relax the configure script so that eventfd can also be detected when cross-compiling. Signed-off-by: Sebastian Kemper <[email protected]>
Well, it's a bank holiday here, so did it already ;-) |
Thank you, Seb. I am merging this PR. |
I am going to drop asterisk-15.x package first in favor of asterisk-16.x package. If asterisk-16.x package has reasonable package sizes and is stable for the production, the I would remove the ast13 as well. |
Maintainer: @jslachta
Compile tested: master, ath79 (musl) + arc (uclibc)
Run tested: ath79 master
Description:
Hello all,
This is not ready for merging yet. There is a problem with segmentation faults when the module loader starts. I've had help from the musl list and raised as issue for it upstream.
According to asterisk wiki Asterisk 15 will be EOL 2019-10-03, so getting Asterisk 16 in would be nice. Not to mention that 16 is an LTS release. But before this can be merged something needs to be done regarding the segmentation fault.
Kind regards,
Seb