[asterisk-dev] [Code Review] 3811: Move main/manager_*.c to loadable modules

Matt Jordan reviewboard at asterisk.org
Thu Jul 17 16:41:44 CDT 2014


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



/trunk/main/stasis_channels.c
<https://reviewboard.asterisk.org/r/3811/#comment23048>

    I'm not sure why these changes (removal of the .to_ami callback) were necessary.
    
    Generally, I prefer the .to_ami callbacks to explicit subscription to message types and construction of messages in the various manager_* modules:
    
    (1) Obtaining the messages in the appropriate modules is done by simply forwarding the topics to the manager topic. That substantially reduces the boilerplate code required.
    
    (2) Co-locating the generation of formatting of messages makes it very easy to update all consumers of a message when a new field is added, helping keep the code/events similar for all consumers of that message.
    
    Generally, I would much prefer these to be kept, and to have the other channel related message have .to_ami callbacks implemented. If anything, the res_manager_channels module should be very small: it should set up a forwarding relationship between the channel topics and the manager topic and be done.


- Matt Jordan


On July 16, 2014, 8:14 p.m., Corey Farrell wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3811/
> -----------------------------------------------------------
> 
> (Updated July 16, 2014, 8:14 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This change moves main/manager_*.c to loadable modules, allowing those events to be disabled by not loading the modules.  This can be accomplished by eventfilter, but eventfilter has a couple issues.  It actually adds more overhead to asterisk since the outbound events must be parsed for each AMI user.  Additionally it causes skips in SequenceNumber, preventing use of that tag to determine if any events were missed during a reconnect.
> 
> Besides converting from built-in units to modules, changes are made to VarSet, ChannelTalkingStart and ChannelTalkingStop.  They no longer use .to_ami callbacks, but instead subscribe to the stasis events like the rest of the res_manager_channels events.  A couple functions were also moved from manager_bridging.c and manager_channels.c to manager.c since they are still needed even if these modules are noload'ed.
> 
> AST_MODULE_INFO_STANDARD for all modules will be updated once r3802 is committed.  This or r3812 will need to be updated depending on which is committed first.
> 
> 
> Diffs
> -----
> 
>   /trunk/main/stasis_channels.c 418738 
>   /trunk/main/manager_system.c HEAD 
>   /trunk/main/manager_system.c 418738 
>   /trunk/main/manager_mwi.c HEAD 
>   /trunk/main/manager_mwi.c 418738 
>   /trunk/main/manager_endpoints.c HEAD 
>   /trunk/main/manager_endpoints.c 418738 
>   /trunk/main/manager_channels.c HEAD 
>   /trunk/main/manager_channels.c 418738 
>   /trunk/main/manager_bridges.c HEAD 
>   /trunk/main/manager_bridges.c 418738 
>   /trunk/main/manager.c 418738 
>   /trunk/include/asterisk/manager.h 418738 
> 
> Diff: https://reviewboard.asterisk.org/r/3811/diff/
> 
> 
> Testing
> -------
> 
> Ran some testsuite's to verify some of the events were still being sent to AMI:
> tests/manager/originate
> tests/apps/channel_redirect
> tests/bridge/connected_line_update
> tests/feature_call_pickup
> tests/apps/dial/dial_answer
> tests/apps/chanspy/chanspy_barge
> tests/funcs/func_push
> 
> This did not provide complete coverage for all effected events, but does verify many events from res_manager_channels.c.  Events from other files were not tested, though res_manager_channels.c was the most likely to cause problems.
> 
> 
> Thanks,
> 
> Corey Farrell
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140717/706923d2/attachment.html>


More information about the asterisk-dev mailing list