[asterisk-dev] [Code Review] rewrite of matching code in chan_sip.c realtime_peer()

[Terry Wilson]

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

Ship it!


This passes my realtime tests. Any behavior changes are due to actual bugfixes. The one caveat I can think of is res_config_curl. All other config backends translate LIKE and wildcard queries to the proper native format. Since we now use a LIKE query in chan_sip where we didn't before, a user of chan_sip with res_config_curl whose HTTP app does not properly handle this situation, might have their setup broken. If the users are using their curl backend for many non-SIP realtime backends, they will have had to already support this. If they were only doing SIP, then they will have some updating to do. Considering the only other way to actually fix the bug would be to do tons of extra queries, I am personally OK with this and am giving this a Ship It. I will hold off on committing this until the end of the week to give asterisk-dev users to discuss this change.

- Terry


On Oct. 13, 2011, 5:32 a.m., wdoekes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1395/
> -----------------------------------------------------------
> 
> (Updated Oct. 13, 2011, 5:32 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> There are several memory leaks in realtime_peer in chan_sip.c. For a thorough explanation, see the bug report.
> 
> Trying to patch those leaks would be making ugly code even uglier. This is an attempt at cleaning up the code a bit.
> 
> I attempted to keep the exact same behaviour as the original code, while increasing readability. (I had to clone the variable gotten from the peerlist to get proper cleanup behaviour.)
> 
> Differences:
> - sippeers ipaddr is now checked even though sippeers host was not matched (I could not make sense of why ipaddr wasn't checked when host was not matched)
> - I don't check sipregs for the insecure column
> - the leaks should be gone ;)
> 
> Up for review here, because the changes are significant enough for me to have made (lots of) errors and because someone might feel that a rewrite is not the way to go.
> 
> 
> This addresses bug ASTERISK-18356.
>     https://issues.asterisk.org/jira/browse/ASTERISK-18356
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/configs/extconfig.conf.sample 340769 
>   /branches/1.8/include/asterisk/config.h 340769 
>   /branches/1.8/main/config.c 340769 
>   /branches/1.8/channels/chan_sip.c 340769 
> 
> Diff: https://reviewboard.asterisk.org/r/1395/diff
> 
> 
> Testing
> -------
> 
> I tested that I got the same behaviour for the tests mentioned in the bug report. The leaks are gone, the bevahour seemed the same.
> 
> I'm too tired to test normal cases right now though.
> 
> 
> Thanks,
> 
> wdoekes
> 
>

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