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

[Mark Michelson]

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


A quick look through and it looks like no behavior has actually been modified, which is exactly what you wanted.

While looking at this patch though, I started getting a bit curious about the origwfmt field for the mohclass structure. Why is it necessary? file.c's streaming functions take care of this for you by using the ast_channel's oldwriteformat field to save off what the format was prior to starting to stream music. Calling ast_stopstream resets the write format to its original value. Is there some subtlety in res_musiconhold that I'm not seeing or is this unnecessary like I think it is?

If it's unnecessary, I may look into making a patch for trunk to get this removed later.


/branches/1.4/res/res_musiconhold.c
<http://reviewboard.digium.com/r/106/#comment591>

    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)


- Mark


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