[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/#review4487
-----------------------------------------------------------



/branches/1.8/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/1395/#comment8669>

    I suppose we could query all insecure sippeers once, then loop through the returned sipregs peerlist checking each name against the insercure sippeers ast_config names. Still ugly, but not nearly as bad as doing a separate query for each.
    
    Here is a patch to your patch that shows a solution: http://paste.ubuntu.com/703664/
    
    Granted, sipregs is ugly and should go away, but if it is possible to fix the bug in 1.8/10 we probably should. I don't like using LIKE with realtime, but we do it many other places and the non-SQL backends already have to handle it.
    
    But, I think this addition allows us to properly handle things without adding any more queries except the multi call to sippeers, but if we really wanted to we could modify the get_insecure_variable_from_sipregs function to return a dup'd var as well as varregs since we've already looked it up. So that extra query could replace the name lookup we'd have to do anyway.



/branches/1.8/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/1395/#comment8667>

    I would expand this to say something like:
    
    The original code was broken. It looked like it would check for insecure sipreg entries, but due to a bug it would never query them. So as not to break backwards compatibility, we do not check it at all here.
    
    or something.



/branches/1.8/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/1395/#comment8668>

    s/need to be having/need to have/


- Terry


On Oct. 5, 2011, 3:01 p.m., wdoekes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1395/
> -----------------------------------------------------------
> 
> (Updated Oct. 5, 2011, 3:01 p.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/channels/chan_sip.c 339564 
>   /branches/1.8/configs/extconfig.conf.sample 339564 
> 
> 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/20111006/9ee1405d/attachment-0001.htm>