[asterisk-dev] [Code Review] 2573: res_parking: Recreate the 'Park' manager action in the new parking system

[Matt Jordan]

> On June 18, 2013, 2:40 p.m., Matt Jordan wrote:
> > /trunk/res/parking/parking_manager.c, line 78
> > <https://reviewboard.asterisk.org/r/2573/diff/3/?file=39822#file39822line78>
> >
> >     "The channel to be called back if the <replaceable>timeout</replaceable> value specified is reached"
> 
> jrose wrote:
>     No, I think that would imply that this is only useful when 'timeout' is provided. This actually applies for regular timeouts as specified by the individual parking lots as well. It is pretty inaccurate for a couple reasons as is though.  For instance, this channel won't necessarily be returned to if comebacktoorigin=no for the parking lot. This was true before as well.
>     
>     Under the changes in https://reviewboard.asterisk.org/r/2638/ this will no longer actually be a channel at all. It might be appropriate to rename this variable. If not, we can simply document it as being the dial string that should be used.

This is an AMI action, not an event passing channel information after a channel has hung up. Since the code already breaks down the channel name into the dial string to dial back if the parked call times out, having the AMI client pass a channel name is somewhat appropriate. In the interest of being backwards compatible where possible, I don't think we should change that unless we have to.

Documentation should still specify exactly what happens and what occurs. So this should still be updated:
"Channel name to use when constructing the dial string that will be dialed if the parked channel times out."

You probably should update the timeout parameter as well to note that it overrides the parking lot's timeout when specified. It's current description doesn't imply that at all.


> On June 18, 2013, 2:40 p.m., Matt Jordan wrote:
> > /trunk/res/parking/parking_manager.c, line 84
> > <https://reviewboard.asterisk.org/r/2573/diff/3/?file=39822#file39822line84>
> >
> >     "The parking lot the channel should be parked in"
> 
> jrose wrote:
>     Oh the dangling preposition wounds my soul.
>     
>     "Which parking lot to use when parking the channel"

"The parking lot to use when parking the channel"


> On June 18, 2013, 2:40 p.m., Matt Jordan wrote:
> > /trunk/res/parking/parking_manager.c, line 88
> > <https://reviewboard.asterisk.org/r/2573/diff/3/?file=39822#file39822line88>
> >
> >     A description should be more verbose than the synopsis.
> >     
> >     Describe:
> >      * The various options and how they behave
> >      * The channel variables set by the AMI action
> >
> 
> jrose wrote:
>     "Park an arbitrary channel with optional arguments for specifying the parking lot used, how long the channel should remain parked, and what dial string to use as the parker if the call times out."

Works for me


- Matt


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


On June 17, 2013, 11:31 p.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2573/
> -----------------------------------------------------------
> 
> (Updated June 17, 2013, 11:31 p.m.)
> 
> 
> Review request for Asterisk Developers, Matt Jordan and rmudgett.
> 
> 
> Bugs: ASTERISK-21641
>     https://issues.asterisk.org/jira/browse/ASTERISK-21641
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This patch gets the 'Park' manager action back up and working. It supports all the arguments to the old park manager action and seems to work in the same way.
> This patch also fixes an issue with the 'Bridge' manager action where channels yanked from two party bridges would leave the bridge up in spite of there being only one channel in the bridge at that point.
> 
> 
> Diffs
> -----
> 
>   /trunk/CHANGES 392115 
>   /trunk/include/asterisk/bridging.h 392115 
>   /trunk/main/bridging.c 392115 
>   /trunk/main/features.c 392115 
>   /trunk/res/parking/parking_manager.c 392115 
> 
> Diff: https://reviewboard.asterisk.org/r/2573/diff/
> 
> 
> Testing
> -------
> 
> Pulled channels from bridged and unbridged calls with the 'Park' and made sure the channel on the other end got torn down in each case. I did similar stuff with the 'Bridge' command.
> 
> 
> Thanks,
> 
> jrose
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130621/329c43b3/attachment.htm>