[asterisk-dev] [Code Review] PreDial - Ability to run dialplan on callee channel and caller channel right before actual Dial

Fri Mar 16 14:00:46 CDT 2012

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

The changes to pbx.c and pbx.h are duplicating a lot of code.  Why is this patch not using the Gosub application to handle the predial execution?  You can now use the relatively new ast_app_run_sub() function to execute the predial dialplan.

A concern that was brought up earlier that has not been addressed is that executing dialplan takes an indeterminate amount of time that could be quite lengthy.  The main loop that parses the various dialed destinations should be broken up into three loops.
for () {
  ast_request()
  callee_predial_gosub()
  ast_call()
}
becomes
for () {
  ast_request()
}
for () {
  callee_predial_gosub()
}
for () {
  ast_call()
}

Also while the loop handling the predial execution is in progress, the caller channel needs to be put into autoservice.

trunk/apps/app_dial.c
<https://reviewboard.asterisk.org/r/1229/#comment10665>

    This isn't following the naming convention of the other flags.  Should be OPT_CALLEE_PREDIAL/OPT_CALLER_PREDIAL the gosub specifier shouldn't be needed because there will not be a macro version.

trunk/apps/app_dial.c
<https://reviewboard.asterisk.org/r/1229/#comment10666>

    Same here.
    OPT_ARG_CALLER_PREDIAL/OPT_ARG_CALLEE_PREDIAL

trunk/apps/app_dial.c
<https://reviewboard.asterisk.org/r/1229/#comment10669>

    This change is not necessary since the flags are not needed outside of this function.

trunk/apps/app_dial.c
<https://reviewboard.asterisk.org/r/1229/#comment10667>

    Why declare a new variable?  Just use chan.

trunk/apps/app_dial.c
<https://reviewboard.asterisk.org/r/1229/#comment10668>

    Why declare a new variable?  Just use tc.

trunk/main/pbx.c
<https://reviewboard.asterisk.org/r/1229/#comment10675>

    Mixing tabs/spaces after comma.

trunk/main/pbx.c
<https://reviewboard.asterisk.org/r/1229/#comment10679>

    This code is only going to remove arguments from the input string if there is a place to put them.  Any arguments should always be removed from the input string so the context,exten,priority can be properly handled.

trunk/main/pbx.c
<https://reviewboard.asterisk.org/r/1229/#comment10677>

    This should be a strrchr in case there are nested parentheses in the argument.

trunk/main/pbx.c
<https://reviewboard.asterisk.org/r/1229/#comment10678>

    format

trunk/main/pbx.c
<https://reviewboard.asterisk.org/r/1229/#comment10674>

    These lines are mixing tabs/spaces for indention.

    Also ipriority is set but not used.  Potential compiler warning.

trunk/main/pbx.c
<https://reviewboard.asterisk.org/r/1229/#comment10676>

    ipriority set but not used.

trunk/main/pbx.c
<https://reviewboard.asterisk.org/r/1229/#comment10671>

    Spaces used instead of tabs for a few lines in this function.
    memset()
    AST_LIST_TRAVERSE_SAFE_BEGIN
    AST_LIST_TRAVERSE_SAVE_END

trunk/main/pbx.c
<https://reviewboard.asterisk.org/r/1229/#comment10672>

    Spaces used instead of tabs for this line.

- rmudgett

On March 15, 2012, 12:49 p.m., kobaz wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1229/
> -----------------------------------------------------------
> 
> (Updated March 15, 2012, 12:49 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> PreDial
>   
>   Say SIP/abc is calling SIP/def
>   You have: Dial(SIP/def)
>   SIP/def-123234 is created.  But how can you tell that from dialplan?
> 
>   You can use a pickup macro: M or U options to Dial(), but you have to wait till pickup to know.
>   'PreDial' new option 'b' to Dial(), will let you run dialplan on the newly created channel before it is connected to the end-device.
> 
>   New way:
>   Dial(SIP/def,,b(predial,s,1))
>   Dialplan will run on SIP/def-123234 and allow you to know right away what channel will be used, and you can set specific variables on that channel.
> 
> You can also run dialplan on the caller channel (option 'B') right before the dial, which is a great place to do a last microsecond UNLOCK to ensure good channel behavior.
> Example:  LOCK(foo)
>           do stuff
>           UNLOCK(foo)
>           Dial(SIP/abc)
> 
> With this above example, say SIP/123 and SIP/234 are running this dialplan.
> 
> SIP/123 locks foo
> SIP/123 unlocks foo
> due to some cpu load issue, SIP/123 takes its time getting to Dial(SIP/abc) and doesn't do it right away
> 
> Meanwhile... SIP/234 zips right by, lock 'foo' is already unlocked, it grabs the lock, does its thing and it gets to Dial(SIP/abc).  SIP/123 wakes up and finally gets to the Dial().  Now you have two channels dialing SIP/abc when there was supposed to be one.
> 
> If your intention is to ensure that Dial(SIP/abc) is only done one at a time, you may have unexpected behavior lurking.
> 
> New way:
>   LOCK(foo)
>   do stuff
>   Dial(SIP/abc,,B(unlock,s,1))
> 
> context unlock {
>   s => {
>     UNLOCK(foo);
>   }
> }
> 
> Now, under no circumstances can this dialplan be run through and execute the Dial unless lock 'foo' is released.
> 
> Obviously this doesn't ensure that you're not calling SIP/abc more than once (you would need more dialplan logic for that), but it will allow a dialplan coder to also put the Dial in the locked section to ensure tighter control.
> 
> 
> Diffs
> -----
> 
>   trunk/apps/app_dial.c 359606 
>   trunk/include/asterisk/pbx.h 359606 
>   trunk/main/pbx.c 359606 
> 
> Diff: https://reviewboard.asterisk.org/r/1229/diff
> 
> 
> Testing
> -------
> 
> context predial {
>   s => {
>     NoOp(I'm Here!);
>   }
> }
> 
>  Dial(SIP/def,,I(predial,s,1))
>    run predial on callee channel
> 
>  Dial(SIP/def&SIP/ghi&SIP/qrx,,I(predial,s,1))
>   runs predial on all three callee channels
> 
>  Dial(SIP/def,,B(predial,s,1))
>    runs predial on caller channel
> 
>  Dial(SIP/def,,B(predial,s,1)I(predial,s,1))
>    runs predial on callee channel and caller channel
> 
> 
> Thanks,
> 
> kobaz
> 
>

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