[asterisk-dev] [Code Review]: Create STACK_PEEK to view calling context, extension, and priority

[Tilghman Lesher]

> On March 8, 2012, 11:54 a.m., Terry Wilson wrote:
> > /trunk/apps/app_stack.c, lines 178-181
> > <https://reviewboard.asterisk.org/r/1776/diff/5/?file=25907#file25907line178>
> >
> >     Document the options passable to 'which'.

Done.


> On March 8, 2012, 11:54 a.m., Terry Wilson wrote:
> > /trunk/apps/app_stack.c, line 621
> > <https://reviewboard.asterisk.org/r/1776/diff/5/?file=25907#file25907line621>
> >
> >     ast_channel_datastore_find() requires channel locking.

Done.  I've also changed all the other places in app_stack which previously used the same code without locking.


> On March 8, 2012, 11:54 a.m., Terry Wilson wrote:
> > /trunk/apps/app_stack.c, line 646
> > <https://reviewboard.asterisk.org/r/1776/diff/5/?file=25907#file25907line646>
> >
> >     coding guidelines (as much as I personally like one-line while statements, they are verboten). Also, I think args.which = ast_skip_blanks(args.which) would be better anyway.

Changed.


> On March 8, 2012, 11:54 a.m., Terry Wilson wrote:
> > /trunk/res/ael/pval.c, line 4650
> > <https://reviewboard.asterisk.org/r/1776/diff/5/?file=25909#file25909line4650>
> >
> >     You have ast_get_context_name possibly returning NULL and then passing that to strcmp without checking for NULL.

It can only return NULL if the context itself is NULL, and that condition is excluded by the enclosing for loop.


> On March 8, 2012, 11:54 a.m., Terry Wilson wrote:
> > /trunk/res/ael/pval.c, lines 4608-4609
> > <https://reviewboard.asterisk.org/r/1776/diff/5/?file=25909#file25909line4608>
> >
> >     I don't like anything about this block. I don't like creating magic contexts/extensions in the first place, and I really don't like including them in every single (ael) context. This very well could be related to my hatred of AEL in general, though, so I am going to have defer reviewing this part of the code to someone who is more familiar with AEL.

I can possibly verify that the context doesn't already exist within AEL and create a random suffix if need be.

In terms of including them in every context, I've designed the code so that it should not matter if it is (making it the last include; verifying that we're within a Gosub stack at call; making the first priority a Goto to a much higher priority, so partial logic isn't appended onto other h extensions.).  Additionally, I don't know of a good way of verifying whether a context is generated from the AEL-macro code or not.  There's nothing in the name, and at this stage, there aren't even extensions in the contexts -- they're waiting for an additional compile step of generating priorities and instantiating.


- Tilghman


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


On March 8, 2012, 1:39 a.m., Tilghman Lesher wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1776/
> -----------------------------------------------------------
> 
> (Updated March 8, 2012, 1:39 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> The issue in question wants variables similar to how Macro worked, which presented variables in the dialplan to denote the calling context, extension, and priority.  This might be similarly set by AEL, except that AEL doesn't have the information to pass at the right time.  It has the information before the call to Gosub is made, but it needs to be passed into the Gosub, so it can be saved on the stack (and thus correctly disappear, restoring previous values, when Return is executed.  Thus, STACK_PEEK was born, with the intention of being able to mimic features in 1.4's AEL that are not present anymore.
> 
> Whether this goes into a version of Asterisk earlier than 11 is a case for the bug reporter to make.
> 
> 
> This addresses bug ASTERISK-19336.
>     https://issues.asterisk.org/jira/browse/ASTERISK-19336
> 
> 
> Diffs
> -----
> 
>   /trunk/apps/app_stack.c 358080 
>   /trunk/funcs/func_dialplan.c 358080 
>   /trunk/res/ael/pval.c 358080 
>   /trunk/tests/test_gosub.c 358080 
>   /trunk/utils/ael_main.c 358080 
>   /trunk/utils/conf2ael.c 358080 
> 
> Diff: https://reviewboard.asterisk.org/r/1776/diff
> 
> 
> Testing
> -------
> 
> Added test cases to test_gosub, in the process discovering another bug that has since been fixed.  Works as designed.
> 
> 
> Thanks,
> 
> Tilghman
> 
>

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