> On July 17, 2014, 5:41 p.m., Matt Jordan wrote: > > /trunk/main/stasis_channels.c, lines 1071-1077 > > <https://reviewboard.asterisk.org/r/3811/diff/1/?file=64565#file64565line1071> > > > > 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.
(1) I couldn't determine what causes the current code (stasis_channels.c / manager_channels.c) to have manager subscribe these events (or not). Maybe I was wrong to assume the existence of .to_ami causes the messages to be broadcast to AMI? If I am wrong then what causes ast_channel_varset_type to be subscribed by the manager topic? (2) As for reducing boilerplate code, I don't understand how this is the case - the new code for these events are almost the same as the old code. Yes (3) The primary goal of this change is to allow res_manager_channels to be excluded from a build, and replaced with something that produces selected events with a different/reduced format, or use other custom filters. I view AMI on two levels - a transport protocol (name value pairs resembling HTTP headers), and an application protocol (the default events produced by Asterisk). Removal of .to_ami isolates the application layer to modules so it can be replaced. For example ast_manager_build_channel_state_string provides 1 field that is useful to me - UniqueID. All other fields are clock cycles wasted during production, transmit and consumption. (4) After this change .to_ami would be dead-code at best when res_manager_channels.so is not loaded. At worst I'm concerned that .to_ami might prevent me from producing custom events. - Corey ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3811/#review12733 ----------------------------------------------------------- On July 16, 2014, 9: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, 9: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
