<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/3089/">https://reviewboard.asterisk.org/r/3089/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On January 3rd, 2014, 11:12 a.m. CST, <b>Mark Michelson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<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...I just don't really like this.
1) Making a macro like this makes the code more esoteric than it previously was.
2) The fact that the loop_obj has to be NULLed out before breaking or returning from the loop if no reference manipulation is required makes it more likely for us to miss referencing errors during review. Plus, those referencing errors would likely be more catastrophic than the previously-missing ao2_iterator_destroy() calls.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">1) RAII_VAR() is esoteric and is now repeatedly abused. The AO2_ITERATOR_SAFE_LOOP_START() / AO2_ITERATOR_SAFE_LOOP_END macros are like the AST_LIST_TRAVERSE_SAFE_BEGIN() / AST_LIST_TRAVERSE_SAFE_END macros.
2) Reference manipulation using ao2_bump() is not a requirement it is an alternate means of achieving the desired result of passing the object reference. More catastrophic means that it is likely to actually get fixed. The missing the ao2_iterator_destroy() calls are also reference leaks.
However, I could require the loop_obj variable declaration be done outside of the iterator loop. It is probably better if it is changed to make the necessary code simpler for the break case and more obvious for the return case.
The AO2_ITERATOR_SAFE_LOOP_START() / AO2_ITERATOR_SAFE_LOOP_END macros or something like them are required in the test_stasis.c unit tests to prevent leaks resulting from the ast_test_validate() macro. (ast_test_validate() really should be renamed to AST_TEST_VALIDATE() to emphasize that it is a macro and that it does not behave like a real function because it has a return in it.)
</pre>
<br />
<p>- rmudgett</p>
<br />
<p>On December 20th, 2013, 4:47 p.m. CST, rmudgett 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.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers.</div>
<div>By rmudgett.</div>
<p style="color: grey;"><i>Updated Dec. 20, 2013, 4:47 p.m.</i></p>
<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;">Due to repeated errors in people remembering to call ao2_iterator_destroy() I have created the AO2_ITERATOR_SAFE_LOOP_START() / AO2_ITERATOR_SAFE_LOOP_END macros. Usage examples are documented where these macros are declared.
</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;">No memory leaks from running "test execute category /stasis/core/ name cache_dump".</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>/branches/12/tests/test_stasis.c <span style="color: grey">(404437)</span></li>
<li>/branches/12/include/asterisk/astobj2.h <span style="color: grey">(404437)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/3089/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>