[asterisk-dev] [Code Review] chan_sip: removing some non-compliant code in v10 and minor fixes

[Terry Wilson]

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



/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/1533/#comment8941>

    I disagree with this code block. Just because the AOR in the To header *typically* differs, it does not mean that it invalid for it not to. It just has to be a valid SIP URI, which means that it doesn't have to have a username part, just the hostport part.
    
    I think we should emulate what we do in check_user_full and set an empty name to the value of the domain so that To: <sip:example.com> matches to peer [example.com]. It is the only way I can think of to logically match these perfectly valid requests.



/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/1533/#comment8938>

    The get_msg_text fixes really aren't related at all to the domain handling fixes. Approval of one shouldn't really affect approval of the other. Also, if one fix needs to be reverted for some reason it makes it a pain to have to re-apply the unrelated fixes in the same patch.
    
    We can go ahead and review them together this time, but in the future it would be better to separate them. If everything is approved, please commit the different sections separately.



/trunk/channels/sip/reqresp_parser.c
<https://reviewboard.asterisk.org/r/1533/#comment8940>

    Same comment here as above. Separate reviews next time, but this time just make sure it is a separate commit.


- Terry


On Oct. 19, 2011, 12:30 p.m., wdoekes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1533/
> -----------------------------------------------------------
> 
> (Updated Oct. 19, 2011, 12:30 p.m.)
> 
> 
> Review request for Asterisk Developers and Paul Belanger.
> 
> 
> Summary
> -------
> 
> See the bug report: there were some XXX'es in the code about code that should be removed.
> 
> The patch does this:
> 
> (1) register_verify won't accept a To: without user-part anymore (illegal according to rfc3261, 10.2)
> 
> (2) check_user_full still doesn't require a user-part, but it won't match usernames by domain anymore. (i.e. it doesn't treat sip:domain as sip:domain at domain anymore)
> 
> (3) there was some freaky logic going on in get_msg_text, I had to rewrite it to make it make sense.
> 
> (4) in the reqresp parser there were lots of if (params) inside a big if (params) block. I scrapped the useless if's.
> 
> 
> This addresses bug ASTERISK-18389.
>     https://issues.asterisk.org/jira/browse/ASTERISK-18389
> 
> 
> Diffs
> -----
> 
>   /trunk/channels/chan_sip.c 341249 
>   /trunk/channels/sip/reqresp_parser.c 341249 
> 
> Diff: https://reviewboard.asterisk.org/r/1533/diff
> 
> 
> Testing
> -------
> 
> It compiles.
> 
> 
> Thanks,
> 
> wdoekes
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20111110/36c8d304/attachment-0001.htm>