> On July 17, 2014, 9: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.
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
stasis mess
age 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.
- Mark
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3811/#review12733
-----------------------------------------------------------
On July 17, 2014, 1:14 a.m., Corey Farrell wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3811/
> -----------------------------------------------------------
>
> (Updated July 17, 2014, 1:14 a.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
