[asterisk-dev] [Code Review] 2511: Add core API for attended transfers and partially hide masquerade usage.

[rmudgett]

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



/team/group/bridge_construction/main/bridging.c
<https://reviewboard.asterisk.org/r/2511/#comment17031>

    Use ARRAY_LEN()



/team/group/bridge_construction/main/bridging.c
<https://reviewboard.asterisk.org/r/2511/#comment17032>

    Where is 2 coming from in this case?


- rmudgett


On May 21, 2013, 3:35 p.m., Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2511/
> -----------------------------------------------------------
> 
> (Updated May 21, 2013, 3:35 p.m.)
> 
> 
> Review request for Asterisk Developers and rmudgett.
> 
> 
> Bugs: ASTERISK-21334 and ASTERISK-21336
>     https://issues.asterisk.org/jira/browse/ASTERISK-21334
>     https://issues.asterisk.org/jira/browse/ASTERISK-21336
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This changeset contains two separate items
> 
> First, there is a new bridge function called ast_bridge_transfer_attended(). In review 2470, this was a stubbed out function, but here, the actual content has been filled in. It performs an attended transfer given the two transferer channels involved. This function is intended to handle cases where both transferer channels are in bridges, or where only one transferer channel is in a bridge. There is currently one piece of functionality missing, and it is dependent on review 2508. I cannot currently perform an attended transfer of a bridge to an unbridged call since I cannot manipulate a local channel in the appropriate way. The addition of the unreal channels in 2508 will allow me to do this.
> 
> Second, the larger of the changes was to go through places where masquerades are performed and eliminate or hide them as much as possible. Two new functions have been created in the channel API, ast_channel_yank() and ast_channel_move(). ast_channel_yank() is intended to be used when code needs to gain local control of a channel in order to redirect it someplace new. ast_channel_move() is intended to be used as a replacement for old uses of ast_channel_masquerade() and ast_do_masquerade() in order to move one channel into where another channel is executing. Neither of these functions should be needed on bridged channels since the bridging API provides ways for channels to move between bridges or perform a task upon exiting a bridge.
> 
> The highlights of this second change include changing the Bridge AMI action and Bridge dialplan applications to no longer require explicit masquerade calls in order to operate. Similarly, the masquerade call made in ast_do_pickup() has changed to use ast_channel_move() instead. ast_async_goto() has been modified to use ast_channel_yank() when redirecting an unbridged channel.
> 
> In the summary for the review, I mentioned that masquerade usage is only partially handled. There are some road blocks preventing complete masquerade hiding:
> 1) The blind transfer and attended transfer code in features.c currently uses masquerades. This code is more-or-less dead at this point, but since the code is still in the tree, it's still using masquerade calls. This code is slated for eventual removal.
> 2) There are a number of channel drivers that make use of ast_channel_masquerade() and ast_channel_transfer_masquerade() in order to perform transfers. These are slated to eventually make use of ast_bridge_transfer_blind() and ast_bridge_transfer_attended() in order to actually perform their transfers.
> 3) There are several channel API calls that check ast_channel_masq(chan) and will call ast_do_masquerade() if the result is non-NULL. Unfortunately, this is still needed because of the channel drivers mentioned in item 2. They simply set up the masquerade and expect the channel thread to actually perform the masquerade.
> 
> Once these three points are addressed, then ast_channel_transfer_masquerade() can be removed entirely, and ast_channel_masquerade() and ast_do_masquerade() can be combined into a single function that simply performs the masquerade.
> 
> As a final note, I also, because I was thinking about these sorts of things, changed the bridge builtin blind transfer code to use ast_bridge_transfer_blind() instead of using what it was previously using.
> 
> 
> Diffs
> -----
> 
>   /team/group/bridge_construction/CHANGES 389354 
>   /team/group/bridge_construction/bridges/bridge_builtin_features.c 389354 
>   /team/group/bridge_construction/channels/chan_mgcp.c 389354 
>   /team/group/bridge_construction/channels/chan_sip.c 389354 
>   /team/group/bridge_construction/include/asterisk/bridging.h 389354 
>   /team/group/bridge_construction/include/asterisk/channel.h 389354 
>   /team/group/bridge_construction/main/bridging.c 389354 
>   /team/group/bridge_construction/main/channel.c 389354 
>   /team/group/bridge_construction/main/features.c 389354 
>   /team/group/bridge_construction/main/pbx.c 389354 
> 
> Diff: https://reviewboard.asterisk.org/r/2511/diff/
> 
> 
> Testing
> -------
> 
> I tested the Manager Bridge action and the Bridge dialplan application with these changes applied. I also did manager redirects of bridged and unbridged channels to ensure they were redirected properly.
> 
> I tested attended transfer code using chan_sip. The chan_sip changes will be in a separate review.
> 
> 
> Thanks,
> 
> Mark Michelson
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130521/5534f7d4/attachment.htm>