[asterisk-dev] [Code Review]: PreDial continuation - Ability to run dialplan on callee and caller channels before Dial

[rmudgett]

> On April 27, 2012, 10:53 a.m., Mark Michelson wrote:
> > This looks good. The only individual comments I have are minor nitpicks.
> > 
> > The biggest issue I have with this patch is that only about a fifth of it actually has anything to do with predial. The rest is about restructuring the chanlist, changing the chanlist to use the list macros, plus various cleanups (moving variable declarations, adding curly braces, deleting trailing whitespace). Please keep your diffs to the point. This would have been much easier and quicker to review if the predial changes were all that were included. If you want to make other changes, do those in a different changeset.

Most of those changes you pointed out are needed for predial.  The chanlist nodes needed new information to break the big for loop into a series-of-three loops.  Breaking the big loop needed some variables to be rearanged.  Converting the chanlist list to use the list macros is technically not needed.  However, I would have had to roll-my-own list node removal in the final loop of the series-of-three loops because the list was open coded.

I backed out the unrelated change to retrydial_exec().


> On April 27, 2012, 10:53 a.m., Mark Michelson wrote:
> > /trunk/apps/app_dial.c, line 2243
> > <https://reviewboard.asterisk.org/r/1878/diff/1/?file=27477#file27477line2243>
> >
> >     Just so people don't look at this and think there's some error, put parentheses around (2 * tech_len) so that it is clear that your intention is only to double it. I had to re-read this to make sure that everything was legit here :)

done


> On April 27, 2012, 10:53 a.m., Mark Michelson wrote:
> > /trunk/apps/app_dial.c, lines 2637-2644
> > <https://reviewboard.asterisk.org/r/1878/diff/1/?file=27477#file27477line2637>
> >
> >     There isn't much point setting DIALEDPEERNUMBER to an empty string. May as well only do it if (!ast_strlen_zero(number))

Actually, I should have set number to NULL.  An empty string would actually put an empty channel var in the list rather than deleting it.

The empty number case is not expected to happen since all outgoing peers had DIALEDPEERNUMBER set before the calls were placed anyway.


- rmudgett


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


On April 23, 2012, 1:19 p.m., rmudgett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1878/
> -----------------------------------------------------------
> 
> (Updated April 23, 2012, 1:19 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This review takes over from the https://reviewboard.asterisk.org/r/1229/ predial review and addresses the concerns brought up by the last review there.
> 
> 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);
>     Return;
>   }
> }
> 
> 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.
> 
> 
> This addresses bug ASTERISK-19548.
>     https://issues.asterisk.org/jira/browse/ASTERISK-19548
> 
> 
> Diffs
> -----
> 
>   /trunk/CHANGES 363306 
>   /trunk/apps/app_dial.c 363306 
> 
> Diff: https://reviewboard.asterisk.org/r/1878/diff
> 
> 
> Testing
> -------
> 
> context predial {
>   s => {
>     NoOp(I'm Here! ARGC=${ARGC} ARG1=${ARG1} ARG2=${ARG2});
>     Return;
>   }
> }
> 
>  Dial(SIP/def,,b(predial^s^1(callee^other)))
>    run predial on callee channel with two specified arguments
> 
>  Dial(SIP/def&SIP/ghi&SIP/qrx,,b(predial^s^1(callee^other)))
>   runs predial on all three callee channels with two specified arguments
> 
>  Dial(SIP/def,,B(predial^s^1(CALLER^extra)))
>    runs predial on caller channel with two specified arguments
> 
>  Dial(SIP/def,,B(predial^s^1(CALLER^extra))b(predial^s^1(callee^other)))
>    runs predial on callee channel with two specified arguments and caller channel with two specified arguments
> 
> 
> Thanks,
> 
> rmudgett
> 
>

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