> 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. > > Russell Bryant wrote: > 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.
And actually, converting this over to optional_api removed the module check anyway - Russell ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3424/#review11562 ----------------------------------------------------------- On April 11, 2014, 10:32 p.m., Russell Bryant wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/3424/ > ----------------------------------------------------------- > > (Updated April 11, 2014, 10:32 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/include/asterisk/beep.h PRE-CREATION > /trunk/funcs/func_periodic_hook.c 412280 > /trunk/apps/app_mixmonitor.c 412278 > /trunk/CHANGES 412278 > > 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 > >
-- _____________________________________________________________________ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
