Review Board 1.7.16


res_musiconhold cleanup

Review Request #2742 - Created Aug. 5, 2013 and updated

wdoekes
trunk
ASTERISK-21775, ASTERISK-22252
Reviewers
asterisk-dev
tilghman
Asterisk
This review concerns cleanup of the res_musiconhold code.

(1) Removed the special REF_DEBUG code which -- as far as I could tell -- only caused problems when enabling REF_DEBUG.
    @tilghman: if the code should still be in there, could you explain why?
    See bug: ASTERISK-22252

(2) Reduce duplicate code, remove unreachable code, remove commented-out code.
   
    Among other things:
    - Removing the HANDLE_REF argument to moh_register(). Doing the unref manually
      takes only one line of code.
    - init_app_class() gets a bit less code, but is now reused by another caller. I
      had to add "XXX: why don't we check for failure (ast_timer_open()) here?"
      because I didn't get why there was a difference.
    - Renamed `mohclass` to `other` in moh_register(). The module won't get an A for
      properly named variables.

(3) Use `init_files_class` instead of `moh_scan_files` directly, because it returns
    <0 for failure and >0 for success and it was only checked for zero (= no files).

(4) Added the patch by Michael Young from ASTERISK-21775, which seems correct to me.

(5) free()'s were encountered that should be ast_free()'s.
    The astobj2.c:162 INTERNAL_OBJ: bad magic number 0xdeaddead for object 0x7faf44978818
    errors I get are not solved, but they're still here when noloading musiconhold,
    so they're unrelated.

(6) Added comment about LOGIC by tilghman:
    - This referred to the previous situation the bare class pointer was compared
      against the state->class pointer. This was changed to take state->name into
      account.
    - When we get to this part, class is reffed, so comparing anything to class should
      be ok, even if state->class is not reffed. Right?

(7) Fixed so realtime cachedrtclasses works with non-files mode.
Used the refcounter(1) to check that things were fine when:
- moh reload'ing
- playing standard musiconhold ("files")
- playing cached RT musiconhold ("files")
- playing uncached RT musiconhold ("files")
- playing standard musiconhold ("quietmp3")
- playing cached RT musiconhold ("quietmp3")
- playing uncached RT musiconhold ("quietmp3")

Tested that the quietmp3 mode re-used the same spawned mpg123 for both realtime MOH classes when CACHERTCLASSES is true.
Total:
3
Open:
3
Resolved:
0
Dropped:
0
Status:
From:
Posted (Aug. 15, 2013, 1:55 p.m.)

   

  
/trunk/res/res_musiconhold.c (Diff revision 2)
 
 
 
class is definitely still reffed and valid given its usage in the calling function.
/trunk/res/res_musiconhold.c (Diff revision 2)
 
 
Red blob.
Posted (Sept. 4, 2013, 3:24 p.m.)

   

  
/trunk/res/res_musiconhold.c (Diff revision 2)
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
In regards to ASTERISK-22252,

I've made a patch which fixes the offending bug without removing the reference debugging code entirely. Unless there is a compelling reason to get rid of it, let's not address that issue in this patch/review.

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.