[asterisk-dev] [Code Review] 3424: mixmonitor: Add option to enable a periodic beep

[Russell Bryant]

> On April 10, 2014, 3:35 p.m., Mark Michelson wrote:
> > /trunk/apps/app_mixmonitor.c, line 1067
> > <https://reviewboard.asterisk.org/r/3424/diff/2/?file=57155#file57155line1067>
> >
> >     Suggestion:
> >     
> >     Instead of doing ast_module_check(), use ast_custom_function_find() to find the PERIODIC_HOOK function. I have a couple of reasons for this:
> >     
> >     1) Currently, if func_periodic_hook.so is loaded, it also means that PERIODIC_HOOK is registered, but that assumption may not always hold.
> >     
> >     2) More importantly, getting the ast_custom_function allows for you to bump the module refcount for func_periodic_hook.so so that the module cannot be unloaded while there is an active mixmonitor. You can decrement the module refcount when the mixmonitor is destroyed.

I started looking at this, but I'm not sure the alternative is better.  They both have downsides.  ast_custom_function_find() may be worse.

Re: #1, you're right, but ...

Re: #2, the module ref count is now always incremented on func_periodic_hook.so when PERIODIC_HOOK() is active.  Also, it looks like ast_custom_function_find() isn't safe.  There is a race condition between finding a function and incrementing the module ref count where the module could be unloaded and cause a crash.  At least with ast_module_check(), ast_func_read() may fail, but it's handled gracefully without crashing.


- Russell


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3424/#review11562
-----------------------------------------------------------


On April 8, 2014, 7:49 p.m., Russell Bryant wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3424/
> -----------------------------------------------------------
> 
> (Updated April 8, 2014, 7:49 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Add an option to enable a periodic beep to be played into a call if it
> is being recorded.  If enabled, it uses the PERIODIC_HOOK() function
> internally to play the 'beep' prompt into the call at a specified
> interval.
> 
> 
> Diffs
> -----
> 
>   /trunk/main/app.c 412023 
>   /trunk/include/asterisk/app.h 412023 
>   /trunk/funcs/func_periodic_hook.c 412023 
>   /trunk/apps/app_mixmonitor.c 412023 
>   /trunk/CHANGES 412023 
> 
> Diff: https://reviewboard.asterisk.org/r/3424/diff/
> 
> 
> Testing
> -------
> 
> exten => 103,1,Answer()
>     same => n,MixMonitor(test.gsm,B(5))
>     same => n,MusicOnHold()
> 
> exten => 104,1,Answer()
>     same => n,MixMonitor(test.gsm,B)
>     same => n,MusicOnHold()
> 
> exten => 105,1,Answer()
>     same => n,MixMonitor(test.gsm,B(3))
>     same => n,StartMusicOnHold()
>     same => n,Wait(15)
>     same => n,StopMusicOnHold()
>     same => n,StopMixMonitor()
>     same => n,Wait(5)
>     same => n,Hangup()
> 
> 
> Thanks,
> 
> Russell Bryant
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140412/fd5195ab/attachment.html>