[asterisk-dev] [Code Review] 3089: Presenting AO2_ITERATOR_SAFE_LOOP_START() for ao2_iterator looping safety.

[Matt Jordan]

> On Jan. 3, 2014, 11:12 a.m., Mark Michelson wrote:
> > 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.
> 
> rmudgett wrote:
>     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.)
>

After looking at the actual change, I'm not sure this is strictly necessary either. I have a few reservations:

(1) I'm not sure emulating AST_LIST macros is necessarily a good idea. AST_LIST happens to have been implemented as a set of macros; ao2 is not. Mixing and matching the conventions isn't something that I'm sure we should encourage.
(2) Hiding a RAII_VAR in the iterator macros creates a hidden cost to using the macros. That alone makes me suspicious of the change, as it means we're replacing one abuse and possibly introducing another.
(3) Fixing reference leaks in test code is good, but it doesn't necessarily require a general purpose construct.

Underlying all of this is some improper usage of RAII_VAR. I'm inclined to think that we should just stop using RAII_VAR indiscriminately and in situations where it makes the code understanding more complex, rather than hide it behind yet another macro.


- Matt


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


On Jan. 3, 2014, 1:15 p.m., rmudgett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3089/
> -----------------------------------------------------------
> 
> (Updated Jan. 3, 2014, 1:15 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> 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.
> 
> 
> Diffs
> -----
> 
>   /branches/12/tests/test_stasis.c 404786 
>   /branches/12/include/asterisk/astobj2.h 404786 
> 
> Diff: https://reviewboard.asterisk.org/r/3089/diff/
> 
> 
> Testing
> -------
> 
> No memory leaks from running "test execute category /stasis/core/ name cache_dump".
> 
> 
> Thanks,
> 
> rmudgett
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140109/ee001ada/attachment.html>