<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 />
<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>
<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/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>