[asterisk-dev] [Code Review] New dialplan function to allow for inheritance of audiohooks

Fri Dec 19 13:36:30 CST 2008

> On 2008-12-18 19:48:35, Russell Bryant wrote:
> > /trunk/funcs/func_audiohookinherit.c, lines 17-19
> > <http://reviewboard.digium.com/r/102/diff/3/?file=1969#file1969line17>
> >
> >     I think this is pretty understood.  I would leave the header unmodified from the standard.

Done

> On 2008-12-18 19:48:35, Russell Bryant wrote:
> > /trunk/funcs/func_audiohookinherit.c, line 26
> > <http://reviewboard.digium.com/r/102/diff/3/?file=1969#file1969line26>
> >
> >     Most files don't have this verbatim tag for the author.  Is it required so that doxygen doesn't complain about the email address?
> 
>  wrote:
>     I just copied the header from app_skel, which had the verbatim tag. Since it's not used elsewhere, I'll just remove it.

Done

> On 2008-12-18 19:48:35, Russell Bryant wrote:
> > /trunk/funcs/func_audiohookinherit.c, line 51
> > <http://reviewboard.digium.com/r/102/diff/3/?file=1969#file1969line51>
> >
> >     While the other sources are pretty obvious, this one isn't so much.  I would add a comment with this that indicates that this applies to the AGC and DENOISE filters.  The others have apps/functions of the same name as the source value supplied here.

Done

> On 2008-12-18 19:48:35, Russell Bryant wrote:
> > /trunk/funcs/func_audiohookinherit.c, line 112
> > <http://reviewboard.digium.com/r/102/diff/3/?file=1969#file1969line112>
> >
> >     Trivial thing .. missing a leading '*' on this line

Oddly, my working copy in the issue13538 branch has the leading '*' but in my working copy of trunk, it did not. This is odd, and I have corrected it.

> On 2008-12-18 19:48:35, Russell Bryant wrote:
> > /trunk/funcs/func_audiohookinherit.c, line 136
> > <http://reviewboard.digium.com/r/102/diff/3/?file=1969#file1969line136>
> >
> >     You have a stray space in this line

done

> On 2008-12-18 19:48:35, Russell Bryant wrote:
> > /trunk/funcs/func_audiohookinherit.c, line 163
> > <http://reviewboard.digium.com/r/102/diff/3/?file=1969#file1969line163>
> >
> >     There is no need for this log message.  ast_calloc() does it for you.

done

> On 2008-12-18 19:48:35, Russell Bryant wrote:
> > /trunk/funcs/func_audiohookinherit.c, line 168
> > <http://reviewboard.digium.com/r/102/diff/3/?file=1969#file1969line168>
> >
> >     Is the channel already locked here?  If not, you need to lock the channel around the datastore add.

done

> On 2008-12-18 19:48:35, Russell Bryant wrote:
> > /trunk/funcs/func_audiohookinherit.c, line 185
> > <http://reviewboard.digium.com/r/102/diff/3/?file=1969#file1969line185>
> >
> >     This isn't needed, ast_calloc() will log errors for you

done

> On 2008-12-18 19:48:35, Russell Bryant wrote:
> > /trunk/funcs/func_audiohookinherit.c, line 225
> > <http://reviewboard.digium.com/r/102/diff/3/?file=1969#file1969line225>
> >
> >     You must lock the channel before datastore_find()

done

> On 2008-12-18 19:48:35, Russell Bryant wrote:
> > /trunk/funcs/func_audiohookinherit.c, line 283
> > <http://reviewboard.digium.com/r/102/diff/3/?file=1969#file1969line283>
> >
> >     You should check the result, but return one of the AST_MODULE_LOAD_* return values explicitly

done

> On 2008-12-18 19:48:35, Russell Bryant wrote:
> > /trunk/include/asterisk/audiohook.h, lines 153-161
> > <http://reviewboard.digium.com/r/102/diff/3/?file=1970#file1970line153>
> >
> >     I would document the channel locking rules with respect to the usage of this function.
> >     
> >     The rest of the datastore API, as far as I know, requires you to do the locking of the channels externally to the function calls.  If this is being used from within do_masq(), then both channels will be locked already, so you're good to go.

done

> On 2008-12-18 19:48:35, Russell Bryant wrote:
> > /trunk/include/asterisk/audiohook.h, line 155
> > <http://reviewboard.digium.com/r/102/diff/3/?file=1970#file1970line155>
> >
> >     You can use the doxygen tag \todo for this

done

> On 2008-12-18 19:48:35, Russell Bryant wrote:
> > /trunk/include/asterisk/audiohook.h, line 162
> > <http://reviewboard.digium.com/r/102/diff/3/?file=1970#file1970line162>
> >
> >     stray space in this function declaration

done

> On 2008-12-18 19:48:35, Russell Bryant wrote:
> > /trunk/main/audiohook.c, lines 450-462
> > <http://reviewboard.digium.com/r/102/diff/3/?file=1971#file1971line450>
> >
> >     Are you sure the channel locking is required here?  Won't they already be locked since this is called from within do_masq()?
> 
>  wrote:
>     You're correct that the channels will be locked from within ast_do_masquerade. I will change this to not include the channel locks in here. Instead, I'll make a note in the doxygen for this function stating that the channels must be locked prior to calling the function (as your comment above suggests I should do).

done

- Mark

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/102/#review257
-----------------------------------------------------------

On 2008-12-18 17:15:30, Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/102/
> -----------------------------------------------------------
> 
> (Updated 2008-12-18 17:15:30)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> It has been a problem for a while now that MixMonitor does not continue to record a conversation if the channel on which the MixMonitor is attached is masqueraded (the most common case being a transfer). MixMonitor is not the only audiohook source that does not propagate to new channels during a masquerade, and so I made the dialplan function generic enough to allow for any audiohook source to be transferred properly.
> 
> This patch introduces a new dialplan function called AUDIOHOOK_INHERIT. The basic mechanism is that if one declares an audiohook from a specific source as being inheritable, then the audiohook will move to the new channel structure during a masquerade. I have included example usage of the function in the XML documentation in the file.
> 
> One point to double check in this is the behavior change I made to ast_do_masquerade. I changed the datastore processing a bit by calling the fixup function only on the datastores on the clone channel and doing so before moving them onto the original channel. As far as I can tell, this actually makes more sense than how it was done previously, but I'd like to know if this could cause troubles.
> 
> I know of one limitation right now, which is that only one audiohook of a given source may be moved to another channel during a masquerade. So if someone had things set up so that there were two MixMonitors on a channel, and that channel got masqueraded into a new one, only the first MixMonitor audiohook found while traversing the list of audiohooks would be moved. At this point, the effort required to accommodate moving multiple audiohooks greatly exceeds the need to implement it, so I am holding back unless it is deemed necessary.
> 
> 
> This addresses bug 13538.
>     http://bugs.digium.com/view.php?id=13538
> 
> 
> Diffs
> -----
> 
>   /trunk/funcs/func_audiohookinherit.c PRE-CREATION 
>   /trunk/include/asterisk/audiohook.h 165810 
>   /trunk/main/audiohook.c 165810 
>   /trunk/main/channel.c 165810 
> 
> Diff: http://reviewboard.digium.com/r/102/diff
> 
> 
> Testing
> -------
> 
> I have tested by using MixMonitor on a channel which gets masqueraded into a new channel as a result of transferring. I have tried both attended and blind transfers, both with the channel as the caller and callee. The result was that the recording continued across the transfer.
> 
> I also did some testing where I set a ChanSpy audiohook on a channel which is masqueraded. I specifically tested this to be sure that the datastore fixup done in app_chanspy would not interfere with the datastore fixup being done in func_audihookinherit. Things worked as expected.
> 
> 
> Thanks,
> 
> Mark
> 
>