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

[wdoekes]

> On Sept. 30, 2011, 2:45 p.m., Terry Wilson wrote:
> > /branches/1.8/channels/chan_sip.c, lines 4635-4642
> > <https://reviewboard.asterisk.org/r/1395/diff/1/?file=19531#file19531line4635>
> >
> >     var is declared twice with two different scopes. Since var is only set non-NULL in the inner scope, the var that is returned will always be NULL.
> >     
> >     The inner declaration should removed and then to be consistent with how most of the code is written, it should just be:
> >     
> >     if ((var = get_insecure_variable_from_config(peerlist))

Indeed. Fixed.


> On Sept. 30, 2011, 2:45 p.m., Terry Wilson wrote:
> > /branches/1.8/channels/chan_sip.c, line 4655
> > <https://reviewboard.asterisk.org/r/1395/diff/1/?file=19531#file19531line4655>
> >
> >     whitespace

Fixed the whitespace.


> On Sept. 30, 2011, 2:45 p.m., Terry Wilson wrote:
> > /branches/1.8/channels/chan_sip.c, lines 4787-4789
> > <https://reviewboard.asterisk.org/r/1395/diff/1/?file=19531#file19531line4787>
> >
> >     This should most likely be ast_load_realtime("sippeers"... since we just checked sipregs and the comment says we are checking sippeers.

Correct. The comment was right, the code was wrong.


> On Sept. 30, 2011, 2:45 p.m., Terry Wilson wrote:
> > /branches/1.8/channels/chan_sip.c, lines 4796-4802
> > <https://reviewboard.asterisk.org/r/1395/diff/1/?file=19531#file19531line4796>
> >
> >     Looking at the original code, it looks like it checks sipregs in a block where realtimeregs is guaranteed to be 0 anyway, so it never would have worked. If this is the case, then there is no functional behavior change and we can remove this comment.

Wrong. The only no-op here is this:
{{{

                                        peerlist = ast_load_realtime_multientry("sippeers", "ipaddr", ipaddr, SENTINEL);
                                        if (peerlist) {
                                                var = get_insecure_variable_from_config(peerlist);
                                                if (var) {
                                                        newpeername = get_name_from_variable(var, newpeername);
this --->                                               varregs = ast_load_realtime("sipregs", "name", newpeername, SENTINEL);
                                                }
                                        }
}}}

The old functionality is this:

IF ANY host=ipaddr
- TRY find_insecure_from sippeers BY host=ipaddr
- TRY find_insecure_from sippeers BY ipaddr=ipaddr
ELSE IF using_sipregs
- TRY find_insecure_from sipregs BY ipaddr=ipaddr
ELSE
- TRY find_insecure_from sippeers BY ipaddr=ipaddr

But I think it should have been this:

IF ANY host=ipaddr
- TRY find_insecure_from sippeers BY host=ipaddr
- IF using_sipregs
- - TRY find_insecure_from sipregs BY ipaddr=ipaddr
- ELSE
- - TRY find_insecure_from sippeers BY ipaddr=ipaddr
ELSE IF using_sipregs
- TRY find_insecure_from sipregs BY ipaddr=ipaddr
ELSE
- TRY find_insecure_from sippeers BY ipaddr=ipaddr

The new code is this:

TRY find_insecure_from sippeers BY host=ipaddr
TRY find_insecure_from sippeers BY ipaddr=ipaddr

The new code probably should do this:

TRY find_insecure_from sippeers BY host=ipaddr
IF using_sipregs
- TRY find_insecure_from sipregs BY ipaddr=ipaddr
ELSE
- TRY find_insecure_from sippeers BY ipaddr=ipaddr

But for backward compatability we must (at least) do this:

TRY find_insecure_from sippeers BY host=ipaddr
IF using_sipregs
- TRY find_insecure_from sipregs BY ipaddr=ipaddr
TRY find_insecure_from sippeers BY ipaddr=ipaddr

The changes against live would be this (when using sipregs):
(a) When any host=ipaddr is found, now sipregs-ipaddr is checked too. <-- probably better
(b) If no host=ipaddr is found, now sippeers-ipaddr is checked too. <-- extra query for every non-matching peer


- wdoekes


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


On Aug. 27, 2011, 3:54 p.m., wdoekes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1395/
> -----------------------------------------------------------
> 
> (Updated Aug. 27, 2011, 3:54 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 333496 
> 
> 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/20111003/55ca0a6e/attachment-0001.htm>