<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/2724/">https://reviewboard.asterisk.org/r/2724/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
<br />
<p>- Matt</p>
<br />
<p>On July 31st, 2013, 8:12 p.m. UTC, opticron wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/static/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers.</div>
<div>By opticron.</div>
<p style="color: grey;"><i>Updated July 31, 2013, 8:12 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-22126">ASTERISK-22126</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
Asterisk
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Fix refcount bugs in the CDR engine relating to use of ao2_iterators.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Ensured the scenario that exposed the bug ran properly and without leaks.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>trunk/main/cdr.c <span style="color: grey">(395852)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/2724/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>