> 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. > > Corey Farrell wrote: > (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. > > Mark Michelson wrote: > I can answer your question from (1). In main/manager_channels.c, there is > the following code: > > manager_topic = ast_manager_get_topic(); > if (!manager_topic) { > return -1; > } > /* lines snipped */ > channel_topic = ast_channel_topic_all_cached(); > if (!channel_topic) { > return -1; > } > > topic_forwarder = stasis_forward_all(channel_topic, manager_topic); > if (!topic_forwarder) { > return -1; > } > > What's happening here is that messages on any channel topic get forwarded > to the manager as a result of the topic forwarder. Since varset messages are > published on a specific channel topic, they get forwarded to the manager, > which calls the to_ami vtable callback to format the message and then send it > out. > > Even without a .to_ami callback, the stasis publication is still > forwarded to the manager topic. The manager topic still gets the message, > sees that there is no to_ami callback and does nothing with the forwarded > message. > > The current use of the topic forwarder in main/manager_channels.c means > that the easiest way I know of to do what you're after (with varset or any > other channel topic publication) is exactly what you've done in this review: > withhold a to_ami callback and create a subscription to the specific event > type in a loadable module. While this is easier to implement, it results in > some extra cycles wasted on forwarding to the manager topic when it really > doesn't need to be done at all. > > The other, more complicated option would be to define the varset message > type in a loadable module. You'd need to create a public function in that > module that is used to publish the message since core modules would not be > able to reliably reference the message type defined in the module. With the > OPTIONAL_API, you can make it so that attempting to call the publication > function provided by the module when the module is not loaded will result in > a no-op. This would make it so the varset message type would only exist if > the appropriate module were loaded. Therefore, attempting to publish a varset > message would be a no-op if the module were not loaded. It also means, > though, that if you are attempting to create your own varset AMI message, you > are on the hook for defining the stasis message type, using the OPTIONAL_API > properly, and defining all callbacks (to_ami, to_json, and to_event) for the > message type. In addition, this sort of behavior is only going to be useful > if the stasi s message type being published is being consumed by AMI. Are there other consumers of the varset message type within Asterisk other than AMI? Could there be? > > I think that, given the options, the way Corey is doing this is the way > to go if we want to make AMI messages for a given message type on a channel > topic provided by a loadable module. > > Corey Farrell wrote: > I'm against use of OPTIONAL_API for this purpose. I've found that > OPTIONAL_API isn't always optional, and I've seen run-time link error's > caused by modules that use "optional" API's from other modules that were not > yet loaded. This leads me to believe that OPTIONAL_API's provided by modules > would not be usable by the core. I have an idea for how to provide a > replacement for OPTIONAL_API in Asterisk 14 as part of the loader.c revamp, > but that doesn't help us now. Additionally manager isn't the only subscriber > to manager_topic, so I think this would cause problems when the module is not > loaded. > > My big complaint with .to_ami is that stasis != AMI. Stasis is the > producer, AMI is a consumer. I think .to_ami is reasonable to use in modules > (like app_queue or res_agi), but not in the Asterisk core. I actually wanted > to strip all .to_ami callbacks from stasis_*.c, I just ran out of time before > the cut-off. I would not be completely against .to_ami if it could be set > after the fact by a module load - if that's possible I'm open to suggestions. > > Based on your comments I'm starting to question the need for: > topic_forwarder = stasis_forward_all(channel_topic, manager_topic); > > If I'm understanding, since no channel topic's have .to_ami, this does > nothing more than waste clock cycles by forwarding topic's that will not be > processed by manager_topic? > > Matt Jordan wrote: > I think you're confusing the point of the to_ami callback. It is not > tightly coupling AMI with Stasis, nor is it implying that AMI is Stasis. > > The purpose of the various callbacks (.to_ami, .to_json, .to_event) is to > provide message formatters. The name "to_ami" is implying that this message > *can* be formatted into AMI syntax. That doesn't mean it has to be > transmitted over AMI, just like being converted into JSON doesn't mean that > it has to be transmitted over ARI. It's simply a way to format the message in > a standard way. > > One of the benefits of this approach is that all code related to the > message and its representation is with the message definition itself. If a > new field is added to the message, it becomes trivial to go through and > update its various formatters. Moving the code into other locations not only > incurs additional penalties (although those penalties are relatively small), > it also requires more work to go find where those events are formatted. In > the case of ARI, for example, this can be slightly non-trivial. > > While I like what you're attempting to do here, I really don't agree with > the approach. Removing formatters still feels like a big step backwards.
I'm making the changes per your request (.to_ami for everything in stasis_channels except ast_channel_snapshot_type). I don't have time to rewrite AMI formatters from the other new modules to .to_ami callbacks. These other modules are just moves from main/manager_*.c to res/res_manager_*.c. As for ast_channel_snapshot_type I suspect this one can't be made into a .to_ami callback since it could potentially result in multiple AMI events. I suspect (hope) I can still create a custom module that replaces res_manager_channels.c, subscribe to the events I want with the formatting I want (ignoring the .to_ami callbacks). If not then this change will not accomplish my goal. It will take me a couple days to test these new changes. As for stasis_channels.c, I think a reorder is appropriate. How would you feel about having the callback procedures (.to_ami, .to_json, .to_event) immediately before each STASIS_MESSAGE_TYPE_DEFN? Also I've seen some cases where the managerEvent XMLDOC is inline, but most cases it is at the top of the file. Based on your comments about keeping things together I think they should be in each .to_ami callback - what do you think? - Corey ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3811/#review12733 ----------------------------------------------------------- On July 21, 2014, 4:56 a.m., Corey Farrell wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/3811/ > ----------------------------------------------------------- > > (Updated July 21, 2014, 4:56 a.m.) > > > Review request for Asterisk Developers. > > > Bugs: ASTERISK-24068 > https://issues.asterisk.org/jira/browse/ASTERISK-24068 > > > 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
