<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/3811/">https://reviewboard.asterisk.org/r/3811/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 17th, 2014, 5:41 p.m. EDT, <b>Matt Jordan</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://reviewboard.asterisk.org/r/3811/diff/1/?file=64565#file64565line1071" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/main/stasis_channels.c</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">STASIS_MESSAGE_TYPE_DEFN(ast_channel_dial_type,</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">1071</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="p">);</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">959</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="p">);</span></pre></td>
</tr>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">1072</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="n">STASIS_MESSAGE_TYPE_DEFN</span><span class="p">(</span><span class="n">ast_channel_varset_type</span><span class="p">,</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">960</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="n">STASIS_MESSAGE_TYPE_DEFN</span><span class="p">(</span><span class="n">ast_channel_varset_type</span><span class="p">,</span></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#ebb1ba" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">1073</font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="p">.</span><span class="n">to_ami</span> <span class="o">=</span> <span class="n">varset_to_ami</span><span class="p">,</span></pre></td>
<th bgcolor="#ebb1ba" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">1074</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="p">.</span><span class="n">to_json</span> <span class="o">=</span> <span class="n">varset_to_json</span><span class="p">,</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">961</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="p">.</span><span class="n">to_json</span> <span class="o">=</span> <span class="n">varset_to_json</span><span class="p">,</span></pre></td>
</tr>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">1075</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="p">);</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">962</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="p">);</span></pre></td>
</tr>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">1076</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="n">STASIS_MESSAGE_TYPE_DEFN</span><span class="p">(</span><span class="n">ast_channel_hangup_request_type</span><span class="p">,</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">963</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="n">STASIS_MESSAGE_TYPE_DEFN</span><span class="p">(</span><span class="n">ast_channel_hangup_request_type</span><span class="p">,</span></pre></td>
</tr>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">1077</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="p">.</span><span class="n">to_json</span> <span class="o">=</span> <span class="n">hangup_request_to_json</span><span class="p">,</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">964</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="p">.</span><span class="n">to_json</span> <span class="o">=</span> <span class="n">hangup_request_to_json</span><span class="p">,</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
<p>On July 17th, 2014, 6:38 p.m. EDT, <b>Corey Farrell</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">(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.</pre>
</blockquote>
<p>On July 18th, 2014, 10:11 a.m. EDT, <b>Mark Michelson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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 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.</pre>
</blockquote>
<p>On July 18th, 2014, 4:08 p.m. EDT, <b>Corey Farrell</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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?</pre>
</blockquote>
<p>On July 29th, 2014, 1:26 p.m. EDT, <b>Matt Jordan</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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?</pre>
<br />
<p>- Corey</p>
<br />
<p>On July 21st, 2014, 4:56 a.m. EDT, Corey Farrell wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers.</div>
<div>By Corey Farrell.</div>
<p style="color: grey;"><i>Updated July 21, 2014, 4:56 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-24068">ASTERISK-24068</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
Asterisk
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/trunk/main/stasis_channels.c <span style="color: grey">(418738)</span></li>
<li>/trunk/main/manager_system.c <span style="color: grey">(HEAD)</span></li>
<li>/trunk/main/manager_system.c <span style="color: grey">(418738)</span></li>
<li>/trunk/main/manager_mwi.c <span style="color: grey">(HEAD)</span></li>
<li>/trunk/main/manager_mwi.c <span style="color: grey">(418738)</span></li>
<li>/trunk/main/manager_endpoints.c <span style="color: grey">(HEAD)</span></li>
<li>/trunk/main/manager_endpoints.c <span style="color: grey">(418738)</span></li>
<li>/trunk/main/manager_channels.c <span style="color: grey">(HEAD)</span></li>
<li>/trunk/main/manager_channels.c <span style="color: grey">(418738)</span></li>
<li>/trunk/main/manager_bridges.c <span style="color: grey">(HEAD)</span></li>
<li>/trunk/main/manager_bridges.c <span style="color: grey">(418738)</span></li>
<li>/trunk/main/manager.c <span style="color: grey">(418738)</span></li>
<li>/trunk/include/asterisk/manager.h <span style="color: grey">(418738)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/3811/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>