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

[Russell Bryant]

> On 2008-12-19 18:40:16, Mark Michelson wrote:
> > 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.

This is a very good point.

I think the reason is that it was just copied from the non-files version of MoH handling.  When an external application is the source for MoH, this is required.  However, it is not required in the files handling version.  So, it can be removed from moh_files_state and the moh_files handling code.

I'll leave it for another patch to remove.


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

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?


- Russell


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