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

[jrose]

> On June 18, 2013, 2:40 p.m., Matt Jordan wrote:
> > /trunk/main/bridging.c, lines 579-580
> > <https://reviewboard.asterisk.org/r/2573/diff/3/?file=39820#file39820line579>
> >
> >     This BUGBUG should still be with the previous function (I think)

I can't honestly say I'm sure either, but I've gone ahead and moved it to be with the stripped out check dissolve code.


> On June 18, 2013, 2:40 p.m., Matt Jordan wrote:
> > /trunk/res/parking/parking_manager.c, lines 572-575
> > <https://reviewboard.asterisk.org/r/2573/diff/3/?file=39822#file39822line572>
> >
> >     This bothers me.
> >     
> >     Different entry points for parking will have different ways of supplying parameters. AMI will be different than dialplan. AGI would be different as well (and there is a long standing patch to have Park added to AGI). Having AMI, AGI, or other interfaces repack their parameters into dialplan syntax feels hackish.
> >     
> >     The parking API should provide a structure that defines the allowed configuration parameters. park_common_setup should take in this structure and configure the channels for parking based on that. It should be up to the various applications to parse out their configuration parameters into the structure and provide it to the common setup routine.

I refactored park_common_setup to be park_application_setup and park_common_setup. park_application setup chops up all of the arguments as it has been doing while park_common_setup actually takes in the individual arguments and parks the call. 


> On June 18, 2013, 2:40 p.m., Matt Jordan wrote:
> > /trunk/res/parking/parking_manager.c, lines 80-82
> > <https://reviewboard.asterisk.org/r/2573/diff/3/?file=39822#file39822line80>
> >
> >     This parameter is optional, so the 'required' attribute should be false

I was sort of operating under the assumption that if required=true weren't applied that required would automatically be false.


> On June 18, 2013, 2:40 p.m., Matt Jordan wrote:
> > /trunk/res/parking/parking_manager.c, lines 77-79
> > <https://reviewboard.asterisk.org/r/2573/diff/3/?file=39822#file39822line77>
> >
> >     This parameter is optional, so the required attribute should be false

This documentation was copied wholesale from the old Park action documentation. I was under the impression that if required weren't included in the parameter that it would automatically not be required, just going on how it gets formatted in the CLI output for the Park docs. It doesn't really seem to change in any perceptible way with required="false" included while included="true" takes away the braces which normally indicate the field is optional.

I'll go ahead and add it for all the relevant fields though.


> 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"

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.


> 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"

Oh the dangling preposition wounds my soul.

"Which 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
> >

"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."


- jrose


-----------------------------------------------------------
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/20130620/a18f2795/attachment-0001.htm>