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

[wdoekes]

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


Changes
-------

Did Terry's suggestion of checking sippeers for insecure anyway.

With the following modifications:
(1) If we're using LIKE here, we better use it for the existing checks as well.
(2) Moved ast_variables_dup() to main/config.c
(3) Fixed leaks and added proper insecure=port check to Terry's example code.
(4) Reused already fetched var which should save us a query (per Terry's suggestion on IRC).
(5) Edited existing function names for consistency.
(6) Trimmed the comments about removing sipregs down to a minimum.

Although it would almost be functioning as promised (apart from the need for the ipaddr column in sippeer which we still need), we have now succeeded in slowing down realtime_peer() even further. But this will be greatly mitigated when/if Terry fixes that 'ast_load_realtime_multientry' returns NULL for zero results.

Testing done: code compiles, no further tests done
http://images.memegenerator.net/instances/400x/9689481.jpg


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

  /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/20111012/55be52fb/attachment-0001.htm>