<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/2285/">https://reviewboard.asterisk.org/r/2285/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On January 26th, 2013, 7:47 a.m., <b>Mark Michelson</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/2285/diff/2/?file=33080#file33080line632" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/channels/chan_gulp.c</a>
<span style="font-weight: normal;">
(Diff revision 2)
</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 int gulp_incoming_request(struct ast_sip_session *session, struct pjsip_rx_data *rdata)</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">632</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">ast_setstate</span><span class="p">(</span><span class="n">session</span><span class="o">-></span><span class="n">channel</span><span class="p">,</span> <span class="n">AST_STATE_RING</span><span class="p">);</span></pre></td>
</tr>
<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">633</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">res</span> <span class="o">=</span> <span class="n">ast_pbx_start</span><span class="p">(</span><span class="n">session</span><span class="o">-></span><span class="n">channel</span><span class="p">);</span></pre></td>
</tr>
<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">634</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></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 started thinking about this this morning and I've realized this could be problematic. If chan_gulp handles an incoming SIP request and starts a PBX thread, then other supplements might run afterwards and do manipulations to the channel.
A possible behavior you might see is that the supplement that extracts callerID might run after chan_gulp's supplement. In the PBX thread, though, the person might call the CALLERID() function in order to try to get or set caller ID on the channel. This presents a conflict.
So on the one hand, we could try to make chan_gulp's supplements be the final ones to be registered so the PBX thread is started as the final step.
On the other hand, having a channel created may be useful for supplements on incoming INVITEs. So maybe PBX thread starting should be its own separate supplement that runs last.</pre>
</blockquote>
<p>On January 27th, 2013, 10:01 p.m., <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;">This is tangentially related to the only design problem I've run into so far: What is an Asterisk thread and what is a pjlib thread?
Consider the following shutdown sequence:
static int unload_module(void)
{
ast_res_sip_destroy_sorcery();
if (monitor_thread) {
stop_monitor_thread();
}
if (memory_pool) {
/* This will cause a crash in Asterisk in debug mode, as the thread
* calling this is not a pjsip thread
*/
pj_pool_release(memory_pool);
}
if (ast_pjsip_endpoint) {
pjsip_endpt_destroy(ast_pjsip_endpoint);
}
pj_caching_pool_destroy(&caching_pool);
return 0;
}
This will crash when compiled in -dev-mode and run in the test suite. The thread shutting down Asterisk is not a pjlib thread, so calls to things like pj_pool_release cause assertion errors. Since I'd prefer that we don't have to register all Asterisk threads with pjlib, we'll need some mechanism to pass information between Asterisk and pjlib that is easily used in a variety of locations (I can imagine AMI/CLI needing this as well). This same pattern - a lack of defined barrier between Asterisk thread actions and pjproject thread actions - occurs within chan_gulp as well.
What I think we'll need to have defined are barriers between where Asterisk interacts with objects and where pjlib interacts. When a channel callback occurs, it should create an object that represents the action it wants a channel servant to act upon and pass it to the channel servant via a thread-safe mechanism. The channel servant will have a registered pjlib thread and act on the action. Going the other direction is generally safer, but we may still want to have a way for channel servants to queue up data for the channel and have a particular set of threads service the channel servants.
The best I can think of for module shutdown is to spawn a thread registered with pjlib, have that thread handle the shutdown, and join on the spawned thread in the module shutdown routine to prevent further shutdown actions from occurring.</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 think separating the act of setting up a channel and starting the PBX is wise and needed. We really just need to use a similar priority system as pjsip modules, with the PBX starting being last as you mentioned.</pre>
<br />
<p>- Joshua</p>
<br />
<p>On January 25th, 2013, 12:26 p.m., Mark Michelson 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 Mark Michelson.</div>
<p style="color: grey;"><i>Updated Jan. 25, 2013, 12:26 p.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 is the first batch of code to review for the new SIP channel driver work. In this, we have the workings of res_sip, the core application-agnostic SIP API, and res_sip_session, the API and workhorse for SIP INVITE sessions. The goal while writing this code was to get into a state where we could run a call through Asterisk using SIP. This code is far from complete, but we reached the milestone of having working incoming and outgoing calls. The calls are currently limited to coming from a hardcoded endpoint and calling out to a hardcoded endpoint, and only ulaw audio is supported.
When you are reviewing this, the feedback we're interested in is the design. Do you foresee troubles with what is here? Is the documentation of the API clear? What we're not interested in at this point are things that are obviously incomplete and nitpicks.
Here's a short list of things we know are in need of improvement:
1) Any place where there is a XXX or TODO comment. In most cases, these are not necessary for reaching the goal of having working calls or are awaiting for sorcery integration.
2) res_sip_endpoint_identifier_constant is not intended to ever actually go into Asterisk. The real endpoint identifiers will use sorcery in order to find endpoints based on data in the SIP request. If the code there is shoddy, it's not a huge deal because it's going to be vaporized eventually.
3) The threadpool infrastructure is present, but it currently is not used. There are two reasons why:
a) It's easy to add in threadpool usage after the fact, given how this is designed.
b) We will need to use a newer version of PJSIP than what currently is in Asterisk.
4) The INVITE session state handler is not especially sophisticated. It currently is a switch statement for different event types, which is just the type of thing that could expand to unreasonable size if not kept in check. An array of callbacks either based on INVITE session state or event type would be much better.
5) The internal functions in res_sip and res_sip_session do not have doxygen yet. They will, eventually.
6) chan_gulp is a working name. It is not necessarily going to be the permanent name for the channel driver.
7) So far, every file that uses PJSIP has required an additional line to be added to the Makefile so that the PJ_FLAGS are added to the _AST_CFLAGS. This probably should be automated in some way so that we do not have to keep adding lines to the Makefile for every file we add.</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;">Joshua and I have been sending calls to Asterisk and receiving calls from Asterisk using chan_gulp. We have also tested error scenarios like calling into a nonexistent extension. </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/channels/Makefile <span style="color: grey">(379431)</span></li>
<li>/trunk/channels/chan_gulp.c <span style="color: grey">(PRE-CREATION)</span></li>
<li>/trunk/include/asterisk/res_sip.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>/trunk/include/asterisk/res_sip_session.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>/trunk/main/taskprocessor.c <span style="color: grey">(379432)</span></li>
<li>/trunk/res/Makefile <span style="color: grey">(379431)</span></li>
<li>/trunk/res/pjproject/Makefile <span style="color: grey">(379431)</span></li>
<li>/trunk/res/pjproject/build.mak.in <span style="color: grey">(379431)</span></li>
<li>/trunk/res/pjproject/pjmedia/build/Makefile <span style="color: grey">(379431)</span></li>
<li>/trunk/res/pjproject/pjsip/build/Makefile <span style="color: grey">(379431)</span></li>
<li>/trunk/res/res_sip.c <span style="color: grey">(PRE-CREATION)</span></li>
<li>/trunk/res/res_sip.exports.in <span style="color: grey">(PRE-CREATION)</span></li>
<li>/trunk/res/res_sip_endpoint_identifier_constant.c <span style="color: grey">(PRE-CREATION)</span></li>
<li>/trunk/res/res_sip_sdp_audio.c <span style="color: grey">(PRE-CREATION)</span></li>
<li>/trunk/res/res_sip_session.c <span style="color: grey">(PRE-CREATION)</span></li>
<li>/trunk/res/res_sip_session.exports.in <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/2285/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>