[asterisk-dev] [Code Review] Fix several bugs with SDP parsing and well-formedness of responses

[mjordan]

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

Ship it!


I'd say ship it given the tests you wrote and their exercising of your changes.  Again, I'm assuming you've also run this through the existing SIP / T38 tests.  The two findings below are really nice to haves or questions.


trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/1516/#comment8726>

    I'm a little confused by this.  Are any of the streams mutually exclusive?  The comment sort of feels like some of them might be, what with it's multiple 'and / or' statements.
    
    If some of these are mutually exclusive, then that doesn't appear to be enforced anywhere.
    
    If none of them are mutually exclusive, then it appears as if the numberofmediastreams checked here should be 4, not 3.  This may also affect your tests.



trunk/channels/sip/include/sip.h
<https://reviewboard.asterisk.org/r/1516/#comment8727>

    It'd be a bit of a pain, but you may want to just renamed the variable as well to reflect its new meaning


- mjordan


On Oct. 12, 2011, 9:17 a.m., opticron wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1516/
> -----------------------------------------------------------
> 
> (Updated Oct. 12, 2011, 9:17 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> Fix bug ASTERISK-16558 which dealt with the order of responses to incoming streams defined by SDP.
> 
> Fix unreported bug where offering multiple same-type streams would cause Asterisk to reply with an incorrect SDP response missing one or more streams without a proper declination.
> 
> Fix bugs related to a single non-audio stream being offered with responses requesting codecs that were not offered in the initial invite along with an additional audio stream that was not in the initial invite.
> 
> 
> This addresses bug ASTERISK-16558.
>     https://issues.asterisk.org/jira/browse/ASTERISK-16558
> 
> 
> Diffs
> -----
> 
>   trunk/channels/chan_sip.c 340161 
>   trunk/channels/sip/include/sip.h 340161 
> 
> Diff: https://reviewboard.asterisk.org/r/1516/diff
> 
> 
> Testing
> -------
> 
> Ran this through a large portion of the external testsuite, but it needs quite a bit more testing before commit.
> 
> 
> Thanks,
> 
> opticron
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20111020/3c2e26fe/attachment.htm>