[asterisk-dev] [Code Review]: Improve SDP parsing warning messages

[Kevin Fleming]

> On March 12, 2012, 9:11 a.m., opticron wrote:
> > 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.
> 
> opticron wrote:
>     The last line of the first paragraph should read "reject the offer in its entirety".

Hmm... if 'continue' drops an 'm' line from the response, that's not good. The only two responses from Asterisk should be rejecting the offer completely (including all 'm' lines present in the offer), or accepting the parts that can be accepted and rejecting those that can't. I understand that this is a long process, but I don't want this patch (or recent patches) to make things worse.

Given that, we should *never* use 'continue' while processing an 'm' line that contains a top-level media type supported by Asterisk (and of course other top-level media types being in the offer should result in a complete rejection of the offer). I'll try to work that into this patch as well.


- Kevin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/1811/#review5792
-----------------------------------------------------------


On March 12, 2012, 7:50 a.m., Kevin Fleming wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1811/
> -----------------------------------------------------------
> 
> (Updated March 12, 2012, 7:50 a.m.)
> 
> 
> Review request for Asterisk Developers and opticron.
> 
> 
> Summary
> -------
> 
> 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.
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/channels/chan_sip.c 358762 
> 
> Diff: https://reviewboard.asterisk.org/r/1811/diff
> 
> 
> Testing
> -------
> 
> Compile testing only.
> 
> 
> Thanks,
> 
> Kevin
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120312/4180b3ef/attachment.htm>