----------------------------------------------------------- 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 > >
-- _____________________________________________________________________ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
