<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/3948/">https://reviewboard.asterisk.org/r/3948/</a>
</td>
</tr>
</table>
<br />
<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 it is kind of hard to review this patch knowing there are potential issues. I'd say that it'd be worthwhile to investigate Walter's potential memory leak before continuing the peer review of this patch.
Beyond that, however, outboundproxy is a mine field. No one has really come to an agreement on what the defined behaviour should be when an outboundproxy is set on a peer - and while dealing with outboundproxy and OPTIONS requests/qualify is slightly less fraught than, say, registration or other scenarios, it bothers me that we're fixing bugs in something that doesn't have a definition of how it is supposed to work.
For reference:
Josh fixing outboundproxy (and breaking other things): https://reviewboard.asterisk.org/r/2851/
Josh asking for clarification on behaviour on the asterisk-dev list (and not getting much discussion): http://lists.digium.com/pipermail/asterisk-dev/2013-October/063312.html
I'd really like to get some consensus on how users of outboundproxy expect it to behave in the scenarios uncovered by ttps://reviewboard.asterisk.org/r/2851/. Otherwise, it feels like we're chasing a moving target here, and that's not fun for anyone. </pre>
<br />
<p>- Matt Jordan</p>
<br />
<p>On August 25th, 2014, 5:04 p.m. CDT, Damian Ivereigh 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 Damian Ivereigh.</div>
<p style="color: grey;"><i>Updated Aug. 25, 2014, 5:04 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-24063">ASTERISK-24063</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;">The outboundproxy setting is ignored when sending the qualify packets (OPTIONS). This means that if an asterisk server is unable to send the packet directly to a peer, it is unable to qualify any non inbound registered peer (e.g. a peer SIP Trunk). This problem is found on asterisk-11.6-cert4 (and many others)
It has been pointed out (thanks Walter Doekes), that the p->outboundproxy may not be freed at the end which would create a memory leak. </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;">Have run this change in production for many months, however the possible memory leak issue needs to be verified.</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>certified/tags/11.6-cert4/channels/chan_sip.c <span style="color: grey">(422052)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/3948/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>