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


The original logic is very twisty, so it is hard to determine whether things are exactly the same. I've stared at this for quite a while and aside from the issues below, things *look* correct. I'll fix things on my side while waiting for you look over things and set up a test box and try hammering different combinations of enabling sipregs, host, ip, insecure, etc. just to make sure that nothing else is broken. The people most likely to use realtime are the larger installs, and I want to make sure we don't break them.


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

    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))



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

    whitespace



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

    whitespace



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

    This should most likely be ast_load_realtime("sippeers"... since we just checked sipregs and the comment says we are checking sippeers.



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

    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.



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

    whitespace



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

    whitespace


- Terry


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/20110930/710aeb23/attachment-0001.htm>