<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/4316/">https://reviewboard.asterisk.org/r/4316/</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 20th, 2015, 9:39 p.m. UTC, <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/4316/diff/2/?file=70752#file70752line149" style="color: black; font-weight: bold; text-decoration: underline;">/branches/13/res/res_pjsip_multihomed.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="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">static pj_status_t multihomed_on_tx_message(pjsip_tx_data *tdata)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">149</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="k">if</span> <span class="p">(</span><span class="n">contact</span> <span class="o">&&</span> <span class="p">(</span><span class="n">PJSIP_URI_SCHEME_IS_SIP</span><span class="p">(</span><span class="n">contact</span><span class="o">-></span><span class="n">uri</span><span class="p">)</span> <span class="o">||</span> <span class="n">PJSIP_URI_SCHEME_IS_SIPS</span><span class="p">(</span><span class="n">contact</span><span class="o">-></span><span class="n">uri</span><span class="p">)))</span> <span class="p">{</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">149</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="k">if</span> <span class="p">(</span><span class="n">contact</span> <span class="o">&&</span> <span class="n">tdata</span><span class="o">-></span><span class="n">msg</span><span class="o">-></span><span class="n">line</span><span class="p">.</span><span class="n">status</span><span class="p">.</span><span class="n">code</span> <span class="o">!=</span> <span class="mi">302</span></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">150</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="o">&&</span> <span class="p">(</span><span class="n">PJSIP_URI_SCHEME_IS_SIP</span><span class="p">(</span><span class="n">contact</span><span class="o">-></span><span class="n">uri</span><span class="p">)</span> <span class="o">||</span> <span class="n">PJSIP_URI_SCHEME_IS_SIPS</span><span class="p">(</span><span class="n">contact</span><span class="o">-></span><span class="n">uri</span><span class="p">)))</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 think there are a couple of problems with this change:
1) You may be trying to access a union member within tdata->msg->line that doesn't make sense here. tdata->msg could be a request or a response. If it is a request, then accessing tdata->msg->line.status is going to get you some goofy values.
2) Adding an exception for 302 responses is not always going to be correct. If we are redirecting to another resource on the same Asterisk server, then we want to rewrite the contact to use the publicly addressable host and port. However, if we are redirecting to a "foreign" URI, then we don't want to rewrite the contact to point back to us.</pre>
</blockquote>
<p>On January 20th, 2015, 9:46 p.m. UTC, <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;">Oh, forgot one more thing
3) If you are going to apply an exception for a 302 response, the same should be done for all 300-class responses as well.</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've been thinking about this more, and you can scrap my finding number 2) here. If people want to "redirect" a device to a resource on the same Asterisk system, they can do that with a Goto in the dialplan, or with the /continue resource in ARI. So anytime you're using an ARI redirect, you should be redirecting to a non-local URI.</pre>
<br />
<p>- Mark</p>
<br />
<p>On January 19th, 2015, 7:17 p.m. UTC, Matt Jordan 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 and Joshua Colp.</div>
<div>By Matt Jordan.</div>
<p style="color: grey;"><i>Updated Jan. 19, 2015, 7:17 p.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-24015">ASTERISK-24015</a>,
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-24703">ASTERISK-24703</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 patch adds a new feature to ARI to redirect a channel to another server, and fixes a few bugs in PJSIP's handling of the Transfer dialplan application/ARI redirect capability.
*New Feature*
A new operation has been added to the ARI channels resource, redirect. With this, a channel in a Stasis application can be redirected to another endpoint of the same underlying channel technology.
- Preemptive question: why 'redirect', and not 'transfer'? Mostly because 'transfer' was always kind of a bad name. If the channel isn't answered, we aren't transferring, we're forwarding. If it is answered, the type of transfer being performed is somewhat vague - is it blind? Is it attended? 'redirect' - while also a slightly loaded term - is a bit more generic and yet descriptive of what is happening: we're redirecting the channel to somewhere else. Answered, not answered, it doesn't matter: your channel is no good here!
*Bug fixes*
In the process of writing this new feature, two bugs were fixed in the PJSIP stack:
(1) The existing .transfer channel callback had the limitation that it could only transfer channels to a SIP URI, i.e., you had to pass 'PJSIP/sip:foo@my_provider.com' to the dialplan application. While this is still supported, it is somewhat unintuitive - particularly in a world full of endpoints. As such, we now also support specifying the PJSIP endpoint to transfer to.
(2) res_pjsip_multihomed was, unfortunately, trying to 'help' a 302 redirect by updating its Contact header. Alas, that resulted in the forwarding destination set by the dialplan application/ARI resource/whatever being rewritten with very incorrect information. Hence, we now don't bother updating an outgoing response if it is a 302. Since this took a looong time to find, some additional debug statements have been added to those modules that update the Contact headers.</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;">Tests were written both for the PJSIP stack as well as the new ARI operation. See https://reviewboard.asterisk.org/r/4352.</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>/branches/13/rest-api/api-docs/channels.json <span style="color: grey">(430794)</span></li>
<li>/branches/13/res/stasis/control.c <span style="color: grey">(430794)</span></li>
<li>/branches/13/res/res_pjsip_transport_websocket.c <span style="color: grey">(430794)</span></li>
<li>/branches/13/res/res_pjsip_nat.c <span style="color: grey">(430794)</span></li>
<li>/branches/13/res/res_pjsip_multihomed.c <span style="color: grey">(430794)</span></li>
<li>/branches/13/res/res_ari_channels.c <span style="color: grey">(430794)</span></li>
<li>/branches/13/res/ari/resource_channels.c <span style="color: grey">(430794)</span></li>
<li>/branches/13/res/ari/resource_channels.h <span style="color: grey">(430794)</span></li>
<li>/branches/13/include/asterisk/stasis_app.h <span style="color: grey">(430794)</span></li>
<li>/branches/13/channels/chan_pjsip.c <span style="color: grey">(430794)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/4316/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>