<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/1811/">https://reviewboard.asterisk.org/r/1811/</a>
</td>
</tr>
</table>
<br />
<p>Ship it!</p>
<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 agree with this patch in that it essentially preserves behavior while making the logic a bit clearer and possibly fixing some cases. process_sdp() probably doesn't understand all possible offer formats, so the lines it doesn't understand are silently dropped (this is bad, but sometimes not avoidable). As that loop is currently written, continues drop media lines from the response while returns reject the response in its entirety.
The tests for this area of code live in tests/channels/SIP/codec_negotiation, but that would best be renamed to include "SDP" since that is its focus.
Things to improve (in a different patch):
Looking at examples in the code, an offer containing a port number of 0 is valid, but must not be used and thus safely ignored even though it should really be represented in the response. Having a transport that is not RTP/AVP or RTP/SAVP should likely be a failing offense or explicit declination since we understand the format of the offer but do not allow that particular media transport (applies to text as well as audio and video). Failure to initialize UDPTL should also most likey be an offer-failing offense or explicit declination. In these cases, we are always capable of failing the entire offer and often unable to decline a single offer because Asterisk's SDP parsing capabilities are so limited.</pre>
<br />
<p>- opticron</p>
<br />
<p>On March 12th, 2012, 7:50 a.m., Kevin Fleming 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 and opticron.</div>
<div>By Kevin Fleming.</div>
<p style="color: grey;"><i>Updated March 12, 2012, 7:50 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 improves the warning messages generated during SDP parsing when various problems are found:
* 'Unsupported media type' is only reported when that is in fact the case, not when a supported media type is included in an 'm' line that has an invalid format.
* All warning messages related to parsing 'm' lines now include the 'm' line contents.
* (minor bugfix) newline added to port-number-zero warning messages.
* Warning messages improved to use RFC-specified terminology for various items.
Kinsey: It's not quite clear to me what the difference between using 'continue' in the 'm'-line processing loop and returning from the function (as is done when a secondary offer an already-seen media type is seen) is; I'm primarily concerned with the effect on the response the Asterisk will generate, because returning from the function would guarantee that our response would *not* include 'm' lines matching each of the 'm' lines in the offer. In addition, we should probably update the testsuite tests that exercise the SDP parser to ensure that these new cases are caught. If you want to give me some pointers on where that stuff lives, I'd be happy to take a stab at it.</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;">Compile testing only.</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/1.8/channels/chan_sip.c <span style="color: grey">(358762)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/1811/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>