[asterisk-dev] [Code Review] 2495: Convert SIP blind transfer code to use core bridging API

[Mark Michelson]

> On May 8, 2013, 1:57 p.m., Matt Jordan wrote:
> > /team/mmichelson/transfer/channels/chan_sip.c, lines 26695-26731
> > <https://reviewboard.asterisk.org/r/2495/diff/1/?file=37148#file37148line26695>
> >
> >     IIRC, the ast_channel must be unlocked prior to call ast_bridge_transfer_blind, as the blind transfer may create a Local channel as part of the blind transfer.
> >     
> >     I'm assuming that nounlock is only non-zero if the channel was already unlocked when we call this. It feels odd to have this, however, when we absolutely must have the channel unlocked prior to calling the blind transfer function.

Yeah, the logic here was presumably as you stated: if *nounlock is already non-zero, then that means we're already working with an unlocked channel and don't need to unlock it ourselves. AFAIK, it isn't actually possible for the *nounlock to be non-zero prior to entering this function, and I believe this is the only place where such an if is used in chan_sip.c. It's more of a case of being overly paranoid than being risky. I chose not to change it because it doesn't do any harm.


- Mark


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


On May 3, 2013, 5:39 p.m., Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2495/
> -----------------------------------------------------------
> 
> (Updated May 3, 2013, 5:39 p.m.)
> 
> 
> Review request for Asterisk Developers and Joshua Colp.
> 
> 
> Bugs: ASTERISK-21519
>     https://issues.asterisk.org/jira/browse/ASTERISK-21519
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This change set converts chan_sip to use the ast_bridge_transfer_blind() API call in order to perform blind transfers and remote attended transfers.
> 
> The majority of this change is stripping logic out of chan_sip that is performed in the core now. The following has all been removed since it is intended to be done in the bridging core now:
> 
> 1. Checks to ensure that the channel is bridged/is in a state where transfers can be performed.
> 2. Any and all parking-related code
> 3. Setting of the BLINDTRANSFER channel variable on involved channels
> 4. CEL events and manager events
> 
> SIP blind transfers and remote attended transfers are still functional. There are some differences between how things previously worked and how they currently work
> 
> 1. Previously, an unbridged channel would reject a REFER request. Now we accept the REFER but send a SIP NOTIFY with sipfrag indicating the transfer failed.
> 2. Some SIP history entries will be different since some processing has been moved to the core. For instance, SIP will not be aware that a transfer to parking occurred, so no such history is logged.
> 3. Previously, channel variables were set on the transferee channel. The clear intention of this was so that when the transferee channel was moved or masqueraded, the variables would be available in the dialplan where the new call was to be made. chan_sip does not have access to the transferee channel now, so instead, it registers a callback to set channel variables on the channel that will end up running dialplan.
> 4. HOLD and UNHOLD frames are not sent to the transferee during a blind transfer. This is done for two reasons. One, the transferee is likely already on hold. Two, the gap between these hold and unhold frames was miniscule and didn't really seem to be there for any purpose.
> 
> 
> Diffs
> -----
> 
>   /team/mmichelson/transfer/channels/chan_sip.c 387543 
> 
> Diff: https://reviewboard.asterisk.org/r/2495/diff/
> 
> 
> Testing
> -------
> 
> Ran several blind transfers both to valid and invalid extensions, and attempted to transfer unbridged calls.
> 
> Transfers to a valid extension worked perfectly.
> Transfers of a bridged call to an invalid extension resulted in the call staying up between the two parties and audio passing properly.
> Transfers of an unbridged call to any extension resulted in the call staying up, with audio still present.
> 
> Signaling was also correct in all situations.
> 
> 
> Thanks,
> 
> Mark Michelson
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130508/55e448c5/attachment-0001.htm>