> 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. > > Corey Farrell wrote: > 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? > > Matt Jordan wrote: > {quote} > 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? > {quote} > > Sounds good to me. > > {quote} > 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? > {quote} > > Some history here: > > When I first wrote the AMI event documentation, there were two problems > that I tried to work around: > (1) The sheer number of AMI events made the scope of the documentation > task rather large > (2) Some AMI events were raised in multiple locations and for different > purposes (and possibly even in separate files) > Both of those reasons led me to allowing for documentation blobs to be > placed immediately before the call to the API function that raised the event. > A normal 'make', however, won't find these documentation strings: it uses awk > and only looks for the first /*** DOCUMENTATION marker; what it finds after > that until the end of the comment is printed out. A 'make full', on the other > hand, will use a pair of python scripts to pull out documentation in the > entire source file. This allowed me to piece together all of the XML > fragments from multiple events and pull them as children into a single > element; it also let me infer parameters from the subsequent API calls. > > So, this: > > /*** DOCUMENTATION > <managerEventInstance> > ... > </managerEventInstance> > ***/ > manager_event(EVENT_FLAG_CALL, "Dial", 2, chans, "SubEvent: > Begin\r\n"...); > > ... > > /*** DOCUMENTATION > <managerEventInstance> > ... > </managerEventInstance> > ***/ > manager_event(EVENT_FLAG_CALL, "Dial", 2, chans, "SubEvent: End\r\n" ...); > > Gets turned into: > > <managerEvent> > <managerEventInstance> > /* Node for dial begin */ > </managerEventInstance> > <managerEventInstance> > /* Node for dial end */ > </managerEventInstance> > </managerEvent> > > There's obvious drawbacks to this approach - it depends on python, a make > target people aren't familiar with, and it builds a bit slower. If you fail > to compile with 'make full', you won't get the documentation you're expecting > to get. > > All of this is a long winded way to say that scattering documentation has > limitations, and doing it the way I did was really a way to work around the > situation at the time. Now, since events are typically generated from Stasis > messages, and we took the approach in 12+ to *not* have lots of events with > subtype fields, it's generally less needed (although it still is used and > exists in certain places). If you think that those drawbacks aren't too big > of a deal, that placing the documentation near the formatting of the > key/value pairs is generally nice (it is really obvious to see when an event > is missing a field)
Sounds like we need to fix the AWK script so it finishes what it starts :) With the possible exception of modules that contain a single application or function, I think all xmldoc blocks should be next to what they are documenting. For now I will only be scattering the documentation for events in stasis_channels.c. - 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
