Review Board 1.7.16


Re-work MoH class object handling

Review Request #106 - Created Dec. 19, 2008 and submitted

Russell Bryant
/branches/1.4
13566
Reviewers
asterisk-dev
Asterisk
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.
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.
Review request changed
Updated (Dec. 19, 2008, 10:37 a.m.)
Posted (Dec. 19, 2008, 12:40 p.m.)
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.
  1. 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.
/branches/1.4/res/res_musiconhold.c (Diff revision 2)
 
 
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)
  1. 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?
  2. 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. :)

https://reviewboard.asterisk.org/ runs on a server provided by Digium, Inc. and uses bandwidth donated to the open source Asterisk community by API Digital Communications in Huntsville, AL USA.
Please report problems with this site to asteriskteam@digium.com.