<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/2381/">https://reviewboard.asterisk.org/r/2381/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 14th, 2013, 11:52 a.m., <b>opticron</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/2381/diff/3/?file=34483#file34483line95" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/main/manager_channels.c</a>
<span style="font-weight: normal;">
(Diff revision 3)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">static struct ast_str *manager_build_channel_state_string(const struct ast_channel_snapshot *snapshot)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">95</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="k">static</span> <span class="kt">void</span> <span class="nf">channel_newexten</span><span class="p">(</span><span class="k">struct</span> <span class="n">ast_channel_snapshot</span> <span class="o">*</span><span class="n">snapshot</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;">This should probably use manager_build_channel_state_string since it's a message being generated from a channel snapshot and it should be processed like the other channel state AMI updates.</pre>
</blockquote>
<p>On March 15th, 2013, 2:56 p.m., <b>David Lee</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;">Newexten has 'Extension' instead of 'Exten'. Is it really worth making that change?
Newexten also has 'Application' and 'AppData' fields, which aren't currently in the channel_state_string. Should we add it for all events that use that function, or just optionally for Newexten?</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;">For Application and AppData, that will depend on whether we consider them critical channel information. If not, it can use the same mechanism as varset and userevent to add non-standard headers to the channel state event.
As for Exten vs Extension, the standard channel event information needs to be consistent across channel state AMI events and I'd go with Extension since there isn't similar abbreviation elsewhere in the message. If we want to be slightly more backwards compatible, the Newchannel event would need an additional/redundant Exten header, but I'd prefer to change it and document that it was changed.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 14th, 2013, 11:52 a.m., <b>opticron</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'll also make the argument that varset and userevent manager events should use manager_build_channel_state_string and the generic mechanism for generating these events since they pull data from a channel state snapshot.</pre>
</blockquote>
<p>On March 15th, 2013, 2:58 p.m., <b>David Lee</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;">Varset is only using the uniqueid and channel name. Userevent is only using the uniqueid. Do we want to add the rest of the channel state to those events?</pre>
</blockquote>
</blockquote>
<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'd say yes since this is channel-related and channel-related events should have at least the base set of channel information.</pre>
<br />
<p>- opticron</p>
<br />
<p>On March 14th, 2013, 11:07 a.m., David Lee wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers.</div>
<div>By David Lee.</div>
<p style="color: grey;"><i>Updated March 14, 2013, 11:07 a.m.</i></p>
<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 patch started out simply as fixing the bouncing tests introduced
in r382685, but required some other changes to give it a decent
implementation.
To fix the bouncing tests, the UserEvent and Newexten AMI events
needed to be refactored to dispatch via Stasis. Dispatching directly
to AMI resulted in those events sometimes getting ahead of the
associated Newchannel events, which would understandably confuse anyone.
I found that instead of creating a zillion different message types and
structures associated with them, it would be preferable to define a
message type that has a channel snapshot and a blob of structured data
with a small bit of additional information. The JSON object model
provides a very nice way of representing structured data, so I went
with that.
* Move JSON support from res_json.c to main/json.c
* Made libjansson-dev a required dependency
* Added an ast_channel_blob message type, which has a channel
snapshot and JSON blob of data.
* Changed UserEvent and Newexten events so that they are dispatched
via ast_channel_blob messages on the channel's topic.
* Got rid of the ast_channel_varset message; used ast_channel_blob
instead.
* Extracted the manager functions converting Stasis channel events to
AMI events into manager_channel.c.</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 bouncing testsuite tests 10 times without failure.</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/json.c <span style="color: grey">(PRE-CREATION)</span></li>
<li>/trunk/main/manager.c <span style="color: grey">(382826)</span></li>
<li>/trunk/main/channel.c <span style="color: grey">(382826)</span></li>
<li>/trunk/configure.ac <span style="color: grey">(382826)</span></li>
<li>/trunk/include/asterisk/autoconfig.h.in <span style="color: grey">(382826)</span></li>
<li>/trunk/include/asterisk/channel.h <span style="color: grey">(382826)</span></li>
<li>/trunk/include/asterisk/manager.h <span style="color: grey">(382826)</span></li>
<li>/trunk/configure <span style="color: grey">(382826)</span></li>
<li>/trunk/apps/app_userevent.c <span style="color: grey">(382826)</span></li>
<li>/trunk/main/manager_channels.c <span style="color: grey">(PRE-CREATION)</span></li>
<li>/trunk/main/pbx.c <span style="color: grey">(382826)</span></li>
<li>/trunk/pbx/pbx_realtime.c <span style="color: grey">(382826)</span></li>
<li>/trunk/res/res_json.c <span style="color: grey">(382826)</span></li>
<li>/trunk/res/res_json.exports.in <span style="color: grey">(382826)</span></li>
<li>/trunk/tests/test_json.c <span style="color: grey">(382826)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/2381/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>