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

[wdoekes]

> On Oct. 12, 2011, 7:47 p.m., Terry Wilson wrote:
> > 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.

You wouldn't happen to have tests using old res_config_sqlite as well? (Which would trigger (ASTERISK-18354 and) ASTERISK-18355.)


> On Oct. 12, 2011, 7:47 p.m., Terry Wilson wrote:
> > /branches/1.8/channels/chan_sip.c, lines 4870-4876
> > <https://reviewboard.asterisk.org/r/1395/diff/5/?file=21209#file21209line4870>
> >
> >     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.

Well.. it could match. If the user joined sipregs.ipaddr into their sippeers view. But now those matches should be done by the new from_sipregs check, so the user won't miss out on results.

I must note that it might be wise to warn the users of the new release that more people can have insecure access now than before. E.g. when someone put the insecure=invite,port in, noticed that it didn't work and didn't remove it afterwards.


- wdoekes


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


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/b71c9d87/attachment-0001.htm>