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

[wdoekes]

> On Oct. 4, 2011, 3:54 p.m., Terry Wilson wrote:
> > /branches/1.8/channels/chan_sip.c, lines 4794-4795
> > <https://reviewboard.asterisk.org/r/1395/diff/2/?file=20972#file20972line4794>
> >
> >     Here we would have varregs set, but no var so we would return incorrectly if we hit this case. Every other case seems to insure that if we set varregs, var is set from sippeers. Seems like here we need another:
> >     
> >             if (!(*var = ast_load_realtime("sippeers", "name", *name, SENTINEL))) {
> >                 ast_variables_destroy(*varregs);
> >                 *varregs = NULL;
> >             }
> >
> 
> Terry Wilson wrote:
>     With the *name = get_name_from_variable(*varregs, *name); first, obviously. :-)

Thanks for catching that. That was quite sloppy of me. You also pointed me to another incompatibility: when sipreg is found but sippeer isn't, the old code would resume trying to find a match.

I added a helper function so the above code can be run from the if-condition.


> On Oct. 4, 2011, 3:54 p.m., Terry Wilson wrote:
> > /branches/1.8/channels/chan_sip.c, lines 4800-4801
> > <https://reviewboard.asterisk.org/r/1395/diff/2/?file=20972#file20972line4800>
> >
> >     s/column/table/
> >     s/field/column/

Fixed.


> On Oct. 4, 2011, 3:54 p.m., Terry Wilson wrote:
> > /branches/1.8/channels/chan_sip.c, line 4807
> > <https://reviewboard.asterisk.org/r/1395/diff/2/?file=20972#file20972line4807>
> >
> >     When I download and apply the patch, it looks like a tab is missing here, though it looks fine on reviewboard. Also, was this supposed to be "better off adding the insecure column to your sippeers table"?

I did find some spaces that should've been tabs, which I removed.

The comment was correct though. I added additional clarification.


- wdoekes


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


On Oct. 3, 2011, 5:36 a.m., wdoekes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1395/
> -----------------------------------------------------------
> 
> (Updated Oct. 3, 2011, 5:36 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/channels/chan_sip.c 338985 
> 
> 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/20111005/90784177/attachment-0001.htm>