[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. 5, 2011, 3:01 p.m.)


Review request for Asterisk Developers.


Changes
-------

Terry and I discussed this issue and the fixes today on IRC. He had noticed that ast_load_realtime_multientry never returns NULL (normally). This means that an entire code avenue was de-facto unreachable -- the one in which sipregs was queried for the insecure column.

This opened up for a number of possible fixes to the problems we were still having: the sippeers table has an 'ipaddr' column that has to match the sipregs 'ipaddr' column for insecure=port to work.

(This is bad for two reasons: (1) the ipaddr should be in sipregs, not in sippeers, (2) needing it in both places breaks the usefulness of sipregs)

Fixing this would involve two changes:
(1) We need to know whether sippeers still has the ipaddr column (or whether sipregs has the insecure column, see point 2). Dynamic (describe table) checking cannot be done in 1.8 and 10 because they are new features (using require_func as test_if_func is not an option). Adding a new sip.conf option seems bloated.
(2) If we check the sipregs for insecure=port, we would either have to do N+1 lookups for every ipaddr that matches (first one multi-lookup on sipregs, then N lookups by name on sippeers for the insecure column) OR we would have to move insecure to sipregs (which is bad design).

You notice that both these changes have dissatisfactory outcomes. And if you add the fact that sipregs really only adds queries (at least +1 query, and possibly more for every find_peer) you may very well come to the conclusion that sipregs should be removed altogether. We did.

I updated the patch to make it more compatible with the old ways -- no extra insecure=port checks on sipregs that got added in diff3. And I added some extra commentary stating that you should avoid using sipregs and use VIEWs instead.


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/channels/chan_sip.c 339564 
  /branches/1.8/configs/extconfig.conf.sample 339564 

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