[asterisk-dev] [Code Review]: F option for bridge application

[jrose]

> On March 22, 2012, 10:46 a.m., Mark Michelson wrote:
> > /trunk/include/asterisk/utils.h, line 432
> > <https://reviewboard.asterisk.org/r/1825/diff/1/?file=26499#file26499line432>
> >
> >     \param, not \brief

Ah, thanks.  Fixed.


> On March 22, 2012, 10:46 a.m., Mark Michelson wrote:
> > /trunk/main/features.c, lines 7707-7709
> > <https://reviewboard.asterisk.org/r/1825/diff/1/?file=26500#file26500line7707>
> >
> >     Doing this is redundant. Both ast_goto_if_exists() and ast_parseable_goto() do the same thing. The difference is that ast_goto_if_exists() takes a separated context, exten, and priority whereas ast_parseable_goto() takes a comma-separated string.
> >     
> >     Take a look at how the F() option is implemented in app_dial().

I should fill you in on why I'm doing this. It's not really redundant, the first goto just reorients the channel.  I was wondering how appropriate it was to handle that this way though.

Because this is a bridge and not a dial or a queue, the channel being connected to doesn't have the same starting point as the channel that called bridge (when a channel is created with dial or queue, it does have the same exten/context/priority). Instead, its context, extension, and priority are all set to whatever it was before hand. The purpose of using goto_if_exists before using ast_parseable goto here is to set the starting point to be the same as the channel that called the bridge.  The alternative would be to check how many arguments there were to OPT_ARG_CALLEE_GO_ON and then manually sort out what is specified and add the leftovers that weren't specified. This is much easier and probably faster as well.


- jrose


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


On March 22, 2012, 9:37 a.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1825/
> -----------------------------------------------------------
> 
> (Updated March 22, 2012, 9:37 a.m.)
> 
> 
> Review request for Asterisk Developers, Mark Michelson and Matt Jordan.
> 
> 
> Summary
> -------
> 
> Similar to the F option added to queue, this adds a new option to the bridge application to transfer the party that gets bridged upon (awkwardly worded maybe) to a position in the dialplan specified by the bridge F option from the perspective of the channel that bridged.
> 
> Also moves a shared function from a couple of places for app_dial and app_queue into utils.
> 
> 
> This addresses bug ASTERISK-19282.
>     https://issues.asterisk.org/jira/browse/ASTERISK-19282
> 
> 
> Diffs
> -----
> 
>   /trunk/apps/app_dial.c 360188 
>   /trunk/apps/app_queue.c 360188 
>   /trunk/include/asterisk/utils.h 360188 
>   /trunk/main/features.c 360188 
>   /trunk/main/utils.c 360188 
> 
> Diff: https://reviewboard.asterisk.org/r/1825/diff
> 
> 
> Testing
> -------
> 
> A sip phone would dial the following extension:
> exten => 045,1,NoOp(BindBape)
> exten => 045,2,Answer()
> exten => 045,3,Set(GLOBAL(CHANBAPE)=${CHANNEL})
> exten => 045,4,Playback(tt-weasels)
> exten => 045,5,Wait(30)
> exten => 045,6,HangUp()
> 
> 
> From here, a second phone would dial any of these extensions:
> exten => 046,1,NoOp(BrideBape)
> exten => 046,2,Answer()
> exten => 046,3,Bridge(${CHANBAPE}, F(5))
> exten => 046,4,Playback(tt-monkeys)
> exten => 046,5,NoOp(Finish!)
> exten => 046,6,NoOp(Finish More!)
> 
> exten => 047,1,NoOp(BridgeBape2)
> exten => 047,2,Answer()
> exten => 047,3,Bridge(${CHANBAPE}, F)
> exten => 047,4,Playback(tt-allbusy)
> exten => 047,5,NoOp(BridgeBape2 finish)
> 
> exten => 048,1,NoOp(BridgeBape3)
> exten => 048,2,Answer()
> exten => 048,3,Bridge(${CHANBAPE}, F(049,4))
> exten => 048,4,NoOp(Failure)
> 
> exten => 050,1,NoOp(BrigeBape4)
> exten => 050,2,Answer()
> exten => 050,3,Bridge(${CHANBAPE}, F(bridgetestcontext,20,1))
> 
> 
> Also this stuff:
> exten => 049,1,NoOp(Failure)
> exten => 049,2,NoOp(Failure)
> exten => 049,3,NoOp(Failure)
> exten => 049,4,Playback(tt-weasels)
> exten => 049,5,NoOp(BridgeBape3 Success)
> 
> [bridgetestcontext]
> exten => 20,1,NoOp(Successful bape4)
> exten => 20,n,Playback(tt-somethingwrong)
> 
> 
> For each of these cases, I just checked to make sure that hanging up the bridger resulted in the bridged party getting sent to the expected position.
> 
> There will be automated tests added for bridging in general as well as for this feature into testsuite soon as well.
> 
> 
> Thanks,
> 
> jrose
> 
>

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