[asterisk-dev] [Code Review] 2797: optional_api: Fix linking problems between modules that export global symbols

[David Lee]

> On Aug. 27, 2013, 5:37 p.m., Corey Farrell wrote:
> > /branches/12/main/loader.c, lines 487-501
> > <https://reviewboard.asterisk.org/r/2797/diff/1/?file=45201#file45201line487>
> >
> >     Instead of removing this section I think it should be enabled for all systems (just remove the #if/#endif lines).  This would ensure required modules get loaded.  I saw on your wiki the section on future work of using MODULEINFO / xmldocs for this.  Knowing dependencies before dlopen() would be better, but I think nonoptreq should deal with it until that change can be made.
> >     
> >     Otherwise if this section of code is being removed then maybe the nonoptreq field should be removed as well.  It's not read from anyplace else in asterisk, only set by some modules.
> 
> David Lee wrote:
>     That was my first thought, too. But nonoptreq lists optional modules,
>     like chan_sip's dependency on res_http_websocket. Then optional
>     modules wouldn't be optional at all, and this patch can become a lot
>     simpler :-D
>     
>     As far as removing the field, I'm not sure of the feasibility of
>     feeding the MODULEINFO information back into the module for its
>     dependencies. If that doesn't work out, we'll need nonoptreq (or
>     something like it) so we can access the dependency information at
>     runtime. Until we know whether or not we need it, I'd rather leave it
>     in there.
> 
> Tilghman Lesher wrote:
>     Actually, nonoptreq is a workaround for a compiler bug, when weak linking was not possible and we needed to enforce load order to ensure modules would link correctly.  As long as the new magic attribute __may_alias__ works the same on all platforms, it's not needed.  That said, I really wouldn't be surprised if it doesn't work the same on all platforms, and the ugly mess that you're trying to eliminate will become necessary again (only a DIFFERENT ugly mess).

may_alias is only needed to avoid type-punning/strict-aliasing
warnings with older GCC's. That magic isn't required for the new
implementation to work.

All the same, I agree that the ugly mess may come back, hence I'm
hesitant about removing the nonoptreq field just yet.


- David


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/2797/#review9533
-----------------------------------------------------------


On Aug. 27, 2013, 12:17 p.m., David Lee wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2797/
> -----------------------------------------------------------
> 
> (Updated Aug. 27, 2013, 12:17 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-22296
>     https://issues.asterisk.org/jira/browse/ASTERISK-22296
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> With the new work in Asterisk 12, there are some uses of the
> optional_api that are prone to failure. The details are rather involved,
> and captured on [the wiki][1].
> 
> This patch addresses the issue by removing almost all of the magic from
> the optional API implementation. Instead of relying on weak symbol
> resolution, a new optional_api.c module was added to Asterisk core.
> 
> For modules providing an optional API, the pointer to the implementation
> function is registered with the core. For modules that use an optional
> API, a pointer to a stub function, along with a optional_ref function
> pointer are registered with the core. The optional_ref function pointers
> is set to the implementation function when it's provided, or the stub
> function when it's now.
> 
> Since the implementation no longer relies on magic, it is now supported
> on all platforms. In the spirit of choice, an OPTIONAL_API flag was
> added, so we can disable the optional_api if needed (maybe it's buggy on
> some bizarre platform I haven't tested on)
> 
> The AST_OPTIONAL_API*() macros themselves remained unchanged, so
> existing code could remain unchanged. But to help with debugging the
> optional_api, the patch limits the #include of optional API's to just
> the modules using the API. This also reduces resource waste maintaining
> optional_ref pointers that aren't used.
> 
> Other changes made as a part of this patch:
>  * The stubs for http_websocket that wrap system calls set errno to
>    ENOSYS.
> 
>  * res_http_websocket now properly increments module use count.
> 
>  * In loader.c, the while() wrappers around dlclose() were removed. The
>    while(!dlclose()) is actually an anti-pattern, which can lead to
>    infinite loops if the module you're attempting to unload exports a
>    symbol that was directly linked to.
> 
>  * The special handling of nonoptreq on systems without weak symbol
>    support was removed, since we no longer rely on weak symbols for
>    optional_api.
> 
>  [1]: https://wiki.asterisk.org/wiki/x/wACUAQ
> 
> 
> Diffs
> -----
> 
>   /branches/12/tests/test_optional_api.c PRE-CREATION 
>   /branches/12/rest-api-templates/swagger_model.py 397673 
>   /branches/12/rest-api-templates/res_ari_resource.c.mustache 397673 
>   /branches/12/res/res_http_websocket.c 397673 
>   /branches/12/res/res_ari_events.c 397673 
>   /branches/12/res/res_ari.c 397673 
>   /branches/12/res/ari/internal.h 397673 
>   /branches/12/res/ari/ari_websockets.c 397673 
>   /branches/12/main/optional_api.c PRE-CREATION 
>   /branches/12/main/loader.c 397673 
>   /branches/12/main/asterisk.c 397673 
>   /branches/12/include/asterisk/optional_api.h 397673 
>   /branches/12/include/asterisk/http_websocket.h 397673 
>   /branches/12/include/asterisk/ari.h 397673 
>   /branches/12/channels/sip/include/sip.h 397673 
>   /branches/12/channels/chan_sip.c 397673 
>   /branches/12/build_tools/cflags.xml 397673 
> 
> Diff: https://reviewboard.asterisk.org/r/2797/diff/
> 
> 
> Testing
> -------
> 
> New optional_api tests pass.
> 
> The config below consistently fails when attempting to connect to the
> ARI WebSocket without this patch; consistently passes with this patch.
> 
> modules.conf:
>     [modules]
>     load => res_stasis.so
>     load => res_ari.so
>     load => res_ari_model.so
>     load => res_ari_events.so
>     load => res_http_websocket.so
> 
> http.conf:
>     [general]
>     enabled=yes
>     bindaddr=127.0.0.1
> 
> ari.conf:
>     [general]
>     enabled = yes
> 
>     [ari]
>     type = user
>     password = ari
> 
> 
> Thanks,
> 
> David Lee
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130828/f298a590/attachment-0001.htm>