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


After fixing res_config_sqlite3 to not strip out the column we are matching on from the resulting varlist for a realtime_multi call, this causes my tests to pass. Just a couple of more comments below. 


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

    whitespace



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

    whitespace



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

    I would go ahead and do the !varregs check anyway, I think. If sipregs is used, the ipaddr will never actually be set in sippeers because the tablename is set to "sipregs" in realtime_update_peer anyway. The only reason we really needed ipaddr in the sippeers table in that case was to avoid getting a warning. It never should have actually matched.


- Terry


On Oct. 12, 2011, 3:20 p.m., wdoekes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1395/
> -----------------------------------------------------------
> 
> (Updated Oct. 12, 2011, 3:20 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/configs/extconfig.conf.sample 340521 
>   /branches/1.8/include/asterisk/config.h 340521 
>   /branches/1.8/main/config.c 340521 
>   /branches/1.8/channels/chan_sip.c 340521 
> 
> 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/20111013/f2272484/attachment-0001.htm>