[asterisk-dev] [Code Review] Re-work MoH class object handling

[Mark Michelson]

> On 2008-12-19 18:40:16, Mark Michelson wrote:
> > /branches/1.4/res/res_musiconhold.c, line 1423
> > <http://reviewboard.digium.com/r/106/diff/2/?file=2072#file2072line1423>
> >
> >     This plus your other changes to astobj2 really aren't necessary. You could just pass NULL here for the callback and it would automatically be set to cb_true (ao2_cb_true as it is renamed in this patch)
> 
>  wrote:
>     Well, that's cool.  :-)  I keep finding myself re-implementing cb_true, which is why I went ahead and exposed it from astobj2.c for common use.
>     
>     I wonder though, do you think it's more understandable to have it this way, or would you rather leave it as passing NULL to get cb_true?

An excellent question, and one I have trouble answering. If the behavior is clearly defined in the ao2_callback() documentation, then I think it's fine to leave as it is. However, converting to making the function publicly available also has the benefit of allowing it to be used outside of ao2_callback(). So you could, for whatever reason, use ao2_cb_true as the default comparison function for ao2_container_alloc() if you really wanted to. While that may sound silly, I actually can think of a potential use of it (though not a high level use). In my ao2_containers branch, I have implemented a linked list ao2 container. If you set the default comparison function to ao2_cb_true, then you could essentially build yourself a LIFO or FIFO structure (stack or queue). Each time you called ao2_find, the top of the stack or front of the queue would be returned.

The tl;dr version is that I'll leave it to your judgment. :)


- Mark


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/106/#review277
-----------------------------------------------------------


On 2008-12-19 16:37:09, Russell Bryant wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/106/
> -----------------------------------------------------------
> 
> (Updated 2008-12-19 16:37:09)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This is a big change to res_musiconhold which re-works how object management was being done for the MoH classes.
> 
> Previously, there was a hand-rolled use count being used.  However, there have been some crashes reported in this module, and I am pretty confident that there are errors in how the reference counts were being handled before.  So, I have reworked this to use astobj2 and made sure that references were handled in all places that they should be.
> 
> There should be no noticeable changes from the user point of view, other than (hopefully) increased stability.  From a developer point of view, this makes the code more streamlined with how we handle reference counts elsewhere in Asterisk.
> 
> Finally, there are some code cleanups mixed in here, too ... sorry, I couldn't help it.
> 
> 
> This addresses bug 13566.
>     http://bugs.digium.com/view.php?id=13566
> 
> 
> Diffs
> -----
> 
>   /branches/1.4/include/asterisk/astobj2.h 166056 
>   /branches/1.4/include/asterisk/strings.h 166056 
>   /branches/1.4/main/astobj2.c 166056 
>   /branches/1.4/res/res_musiconhold.c 166056 
> 
> Diff: http://reviewboard.digium.com/r/106/diff
> 
> 
> Testing
> -------
> 
> I have made a bunch of calls to MoH.  I have done a bunch of reloads.  I did reloads adding and removing classes.  I also made sure that I did a reload to remove a class that was in use.
> 
> During these tests, I made sure that classes got destroyed when they were supposed to, and that there were no memory leaks.  All looks good.
> 
> 
> Thanks,
> 
> Russell
> 
>