[asterisk-dev] [Code Review] 2724: Fix memory leaks in the CDR engine

[Matt Jordan]

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


So, the two missed deref's on the iterator are bad. Sorry :-)

However, I don't like the pattern using RAII_VAR here for two reasons:
(1) It adds complexity where it isn't needed. RAII_VAR is immensely useful when control flow can break in multiple places. That isn't the case with I think any of the loops here.
(2) It is "clever", but hides the meaning of what is going on. If we want to be explicit in what is being cleaned up, a for loop is much better:

for (; cdr = ao2_iterator_next(&it_cdrs); ao2_ref(cdr, -1)) {
   ...
}

Changing things to scoped locks has the same problem. Scoped locks are awesome, but again, only necessary when you have either:
(1) A small section to protect that can be nested in a block
(2) Multiple control break points

Again, most of the places that you changed that here, that isn't the case. I'm less adverse to this change, but I'm not sure it's really all that useful or needed.

- Matt Jordan


On July 31, 2013, 8:12 p.m., opticron wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2724/
> -----------------------------------------------------------
> 
> (Updated July 31, 2013, 8:12 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-22126
>     https://issues.asterisk.org/jira/browse/ASTERISK-22126
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Fix refcount bugs in the CDR engine relating to use of ao2_iterators.
> 
> 
> Diffs
> -----
> 
>   trunk/main/cdr.c 395852 
> 
> Diff: https://reviewboard.asterisk.org/r/2724/diff/
> 
> 
> Testing
> -------
> 
> Ensured the scenario that exposed the bug ran properly and without leaks.
> 
> 
> Thanks,
> 
> opticron
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130731/757dafa2/attachment.htm>