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

[rmudgett]

> 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.

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.)


- rmudgett


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


On Dec. 20, 2013, 4:47 p.m., rmudgett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3089/
> -----------------------------------------------------------
> 
> (Updated Dec. 20, 2013, 4:47 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 404437 
>   /branches/12/include/asterisk/astobj2.h 404437 
> 
> 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/20140103/5a8cf2bf/attachment-0001.html>