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

[Terry Wilson]

> On Oct. 20, 2011, 11:24 a.m., David Vossel wrote:
> > /trunk/channels/chan_sip.c, lines 16010-16022
> > <https://reviewboard.asterisk.org/r/1533/diff/1/?file=21264#file21264line16010>
> >
> >     We can't remove this.  There is a reason that comment was added years ago instead of actually fixing the problem.  Nothing about this has changed, people still depend on this matching to work the way it always has.

We sometimes have had a tendency in the past to value "backwards compatibility" even in cases where no device has ever been seen to exhibit the behavior we see that should be changed. An example would be the registertrying option that was added which never actually worked because someone thought that there might possibly be a client that would require a 100 reply before getting the final response to a REGISTER request. I don't think we can rely on the fact that it wasn't fixed at the time as indicating that it shouldn't have been, but just that someone was being very cautious.

With that said, though, If we do get a request with something like From: <sip:example.com> or From: <sip:hostname>, I don't see a problem with trying to match it against peers of the same name. We are essentially just trying to match the peer name by username then by hostname. I think this actually makes a lot of sense (or at least as much as matching things based on the From header in general does). If it were me, I would just re-word the comment to make it clear that we are matching the peername by whatever information we can find and remove the XXX and complaint about it being non-standard. It isn't non-standard because we are free to match the request to an internal peer however we want. If we want to have peer names like [bob.com] (and we sometimes do), that is perfectly fine.


- Terry


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


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/20111109/462ad182/attachment-0001.htm>