[asterisk-dev] [Code Review]: Improvements to Asterisk parking (Repost of review 963)

[Mark Michelson]

> On Jan. 19, 2012, 6:26 p.m., rmudgett wrote:
> > /trunk/configs/features.conf.sample, line 53
> > <https://reviewboard.asterisk.org/r/1674/diff/2/?file=23487#file23487line53>
> >
> >     Code sets PARKER which actually is the channel that parked the call.  (As best as the code can tell anyway.)

You're right that the code is still setting the "PARKER" variable. However, the code is actually setting the parked channel and not the one doing the parking, so I'll get the variable name straightened out.


> On Jan. 19, 2012, 6:26 p.m., rmudgett wrote:
> > /trunk/main/features.c, lines 5476-5480
> > <https://reviewboard.asterisk.org/r/1674/diff/2/?file=23488#file23488line5476>
> >
> >     This warning needs to be reworded.
> >     "Parking lot %s needs comebackcontext\n"
> >     
> >     The code does not set to default.
> >     
> >     The code should set error = -1 to disable the parking lot until the user fixes the config error.

I don't like saying it "needs a comebackcontext."

This patch is written so as not to force people who are already setting comebacktoorigin=no to have to configure each of their parking lots to have a comebackcontext. They'll continue to function with the default "parkedcallstimeout" context they would have gone to. They don't actually need to set the option for it to work. The only way that ast_strlen_zero(cfg->comebackcontext) will evaluate true is if they specifically set "comebackcontext=" in their config, which would be bizarre. So that's why I worded it as saying that the context was left blank.

The message as I have it is grammatically awful, so I'll fix that. I'll keep it so it does not set the default value, and I will set error to -1.


- Mark


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


On Jan. 19, 2012, 5:54 p.m., Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1674/
> -----------------------------------------------------------
> 
> (Updated Jan. 19, 2012, 5:54 p.m.)
> 
> 
> Review request for Asterisk Developers, rmudgett and Mitch Sharp.
> 
> 
> Summary
> -------
> 
> Many moons ago, Mitch Sharp posted some updates for Asterisk parking (https://reviewboard.asterisk.org/r/963/). Here is the copy and pasted text from that review description:
> ------
> I posted a patch to issues a few weeks ago that duplicated some existing features.  (https://issues.asterisk.org/view.php?id=17947)
> 
> I have reworked it to extend the existing parking behavior.
> 
> This patch does the following:
> - Make comebacktoorigin a per-parkinglot option.
> - Make parkedmusicclass a per-parkinglot option, and make sure parkinglot->parking_con_dial gets set to "park-dial" for additional parking lots instead of being left NULL (already have a patch in issues for this for as a fix for 1.8.0-beta4, https://issues.asterisk.org/view.php?id=17946)
> - Adds a per-parkinglot option comebackcontext, which defaults to the usual 'parkedcallstimeout' context.  Allows the user to handle timed out calls for different parking lots in different contexts.
> - Adds a per-parkinglot option comebackdialtime, which allows user to override the dial time specified in automatically created extensions when comebacktoorigin=yes.
> - Adds channel variable PARKER to timed out calls, so user can easily dial the original device that parked the call if comebacktoorigin=no.
> - Update CLI command 'features show' to include the comebacktoorigin, comebackcontext and comebackdialtime values.
> - Updated /configs/features.conf.sample
> 
> This patch doesn't change any default behavior.
> 
> Looking forward to some feedback.
> ------
> 
> It got some review feedback, but there was never a new version of the patch posted. I've taken the original patch and updated it to apply to Asterisk trunk as it is now.
> 
> As far as I can tell, I've got the patch behaving exactly how it was as originally written. There is a spot that seems odd to me, and I'll highlight it in the diff.
> 
> I've included Richard in this review since he knows parking a lot better than most people do at the moment since he did some refactoring of it recently. 
> 
> 
> This addresses bug ASTERISK-16643.
>     https://issues.asterisk.org/jira/browse/ASTERISK-16643
> 
> 
> Diffs
> -----
> 
>   /trunk/configs/features.conf.sample 351309 
>   /trunk/main/features.c 351309 
> 
> Diff: https://reviewboard.asterisk.org/r/1674/diff
> 
> 
> Testing
> -------
> 
> I have set up parking lots and ensured that the new options take effect when set and that the defaults are used when not set.
> 
> 
> Thanks,
> 
> Mark
> 
>

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