[asterisk-dev] [Code Review] 2511: Add core API for attended transfers and partially hide masquerade usage.
rmudgett
reviewboard at asterisk.org
Fri May 10 12:53:07 CDT 2013
> On May 9, 2013, 11:51 p.m., rmudgett wrote:
> > /team/mmichelson/transfer/bridges/bridge_builtin_features.c, line 176
> > <https://reviewboard.asterisk.org/r/2511/diff/1/?file=37436#file37436line176>
> >
> > This callback needs to know if it is redirecting a channel or the bridge. If you are redirecting a channel, you don't want to do the data copy. If you are transferring the bridge, then you do want to copy the data.
> >
> > The called party of a bridge transfer is going to see the transferrer as the calling party and be slightly surprised when he finds out that the caller is not there with the parties in the bridge. In this case though what caller information can be used since an ad-hock bridge has some difficulty in knowing its party id.
>
> Mark Michelson wrote:
> I've been giving this some thought. The reason this callback is here is because originally, the blind transfer code called dial_transfer() in order to create a local channel and dial the extension and context. dial_transfer() does this same copying of information. dial_transfer() is also called for the builtin attended transfer code. For attended transfers, since the call is being made from the transferer to the transfer target, the copying of caller data to the outbound local channel makes sense.
>
> For blind transfers, I think that copying datastores and channel variables really just does not make sense at all since the call is not supposed to be coming from the transferer channel. In addition, I think the connected line information for the outbound call should be handled by the bridge core instead of in a callback.
>
> In other words, I think the callback should be removed entirely. What do you think?
In general, I think the callback still should indicate if the transfer is a redirect or bridge transfer anyway. It seems like a useful piece of information the callback may need.
We still should copy channel variables and datastores from the transferrer channel. We just need to determine what to use as a caller-id to send out. In this case, the callback is not needed because it seems like a general requirement for blind transferring a bridge.
What COLP should the bridge supply? For two-party bridges it is easy: the caller-id of the peer. For multi-party bridges, that is not defined yet. How can a multi-party bridge get a COLP identity representing the members of the bridge? For ConfBridges, it easy since the bridge profile could be expanded to include an identity. Ad-hock multi-party bridges don't have an obvious way to get an identity. We could define bridge identities for specific scenarios: "Bridge Transfer" <???>, "Multi-party" <???>. We could take the caller-id of the oldest channel in the bridge.
> On May 9, 2013, 11:51 p.m., rmudgett wrote:
> > /team/mmichelson/transfer/bridges/bridge_builtin_features.c, lines 202-203
> > <https://reviewboard.asterisk.org/r/2511/diff/1/?file=37436#file37436line202>
> >
> > The DTMF blind transfer has a feature that if the channel variable GOTO_ON_BLINDXFR exists on the transferrer channel after a blind transfer, the channel goes off and runs at that location. This looks like a job for the after bridge goto/callback on the transferrer channel and a BUGBUG note to do it.
>
> Mark Michelson wrote:
> If I set the after bridge goto, then why would a BUGBUG note be necessary?
It should have said or not and. The BUGBUG was in the case you wanted to defer implementation of the GOTO_ON_BLINDXFER.
> On May 9, 2013, 11:51 p.m., rmudgett wrote:
> > /team/mmichelson/transfer/main/bridging.c, lines 1986-1993
> > <https://reviewboard.asterisk.org/r/2511/diff/1/?file=37439#file37439line1986>
> >
> > This isn't strictly necessary. The chan pointer never changes for a bridge_channel.
>
> Mark Michelson wrote:
> Which isn't necessary, the lock or the NULL check? Or both?
Never mind. It is safer to do this when it is not done by a bridging thread. The bridge_channel->chan pointer is valid as long as the channel is in the bridge.
> On May 9, 2013, 11:51 p.m., rmudgett wrote:
> > /team/mmichelson/transfer/main/bridging.c, lines 5142-5144
> > <https://reviewboard.asterisk.org/r/2511/diff/1/?file=37439#file37439line5142>
> >
> > If you invert the ternary expression, you won't need the "== 0".
> >
> > Unfortunately, just calling ast_bridge_merge() is not going to work if one of the bridges is a parking lot. In that case you must use a local channel and make the ;2 channel join with swap. Then kick out the other transferrer channel from the other bridge.
>
> Mark Michelson wrote:
> All right. Is there an existing bridge flag (e.g. DONT_MERGE_ME_BRO) that I can check so that I can know if a bridge merge is not doable? Or am I going to need to create a flag for this purpose?
>
> The operation you're describing is going to be very similar to the operation that will be performed in attended_transfer_bridge() once it is doable.
I am thinking about what the unreal channel optimization code has to check to determine which way a bridge channel move/swap or bridge merge needs to be done. It would be really simple to create a local;1--local;2 channel pair to swap with the two transferrer channels and let the channel optimization code take care of it.
> On May 9, 2013, 11:51 p.m., rmudgett wrote:
> > /team/mmichelson/transfer/main/features.c, lines 7217-7223
> > <https://reviewboard.asterisk.org/r/2511/diff/1/?file=37441#file37441line7217>
> >
> > This comment is not necessary. Moving a channel from one bridge to another does not need nor should it mess with the ast_bridge_features. The bridge features are needed when the channel first enters the bridging system.
> >
> > Interval hooks need to be rethought and we need to add an API call to put new hooks on channels while in a bridge.
>
> Mark Michelson wrote:
> Okay, what I can do is shrink this down to a BUGBUG comment that just mentions that the bridge features specified in the Bridge application will not be present.
Only the interval hook will currently have no effect. The pre_bridge_call() code that you extracted from ast_bridge_call() sets up the datastore for the DTMF features and they will take effect when the channel is moved.
> On May 9, 2013, 11:51 p.m., rmudgett wrote:
> > /team/mmichelson/transfer/main/features.c, lines 7365-7392
> > <https://reviewboard.asterisk.org/r/2511/diff/1/?file=37441#file37441line7365>
> >
> > You must explicitly destroy the bridge on failure for these two exits:
> > ast_bridge_destroy(bridge);
> >
> > Also playtone was only played for channel2 before not for both channels.
>
> Mark Michelson wrote:
> Yes, I purposely changed the tone to be played to both channels since it did not make sense to only play it to one.
What? Trying to remove the distinction between channel1 and channel2? :) Actually, it does make sense to play it to only one side when you use this AMI action to implement your own call transfers.
- rmudgett
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/2511/#review8569
-----------------------------------------------------------
On May 8, 2013, 6:36 p.m., Mark Michelson wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2511/
> -----------------------------------------------------------
>
> (Updated May 8, 2013, 6:36 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/mmichelson/transfer/bridges/bridge_builtin_features.c 388000
> /team/mmichelson/transfer/include/asterisk/bridging.h 388000
> /team/mmichelson/transfer/include/asterisk/channel.h 388000
> /team/mmichelson/transfer/main/bridging.c 388000
> /team/mmichelson/transfer/main/channel.c 388000
> /team/mmichelson/transfer/main/features.c 388000
> /team/mmichelson/transfer/main/pbx.c 388000
>
> 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/20130510/ed1cbc4f/attachment-0001.htm>
More information about the asterisk-dev
mailing list