[asterisk-dev] [Code Review] 3891: Fix Lua regression caused by me fixing ASTERISK-23818 incorrectly.

[Matt Jordan]

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


This isn't entirely germane to your patch, but I figured I'd comment on this first:

{quote}
For later...  "dialplan show" always shows extensions, includes, then switches but pbx_find_extension actually processes extensions, switches then includes.  Which is correct?
{quote}

Per the documentation, it should be exact extensions, pattern match extensions, includes, then switches. To quote the sample file:

{quote}
; Contexts contain several lines, one for each step of each extension.  One may
; include another context in the current one as well, optionally with a date
; and time.  Included contexts are included in the order they are listed.
; Switches may also be included within a context.  The order of matching within
; a context is always exact extensions, pattern match extensions, includes, and
; switches.  Includes are always processed depth-first.  So for example, if you
; would like a switch "A" to match before context "B", simply put switch "A" in
; an included context "C", where "C" is included in your original context
; before "B".
{quote}

I'm hesitant to advocate changing the code, simply because behaviour changes in dialplan have unforeseen ripple effects. Often, people who have encountered "quirks" will have already worked around them, and by fixing the quirks we create more breakage than we'd like. If we do decide to tweak the matching behaviour, I'd go with a "trunk only" patch - even if the patch makes the behaviour more consistent with the documentation.

- Matt Jordan


On Aug. 5, 2014, 7:50 p.m., George Joseph wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3891/
> -----------------------------------------------------------
> 
> (Updated Aug. 5, 2014, 7:50 p.m.)
> 
> 
> Review request for Asterisk Developers and Matt Jordan.
> 
> 
> Bugs: ASTERISK-23818
>     https://issues.asterisk.org/jira/browse/ASTERISK-23818
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> ASTERISK-23818 (lua contexts being overwritten by contexts of the same name in pbx_config) surfaced because pbx_lua, having the AST_MODFLAG_GLOBAL_SYMBOLS set, was always force loaded before pbx_config.  Since I couldn't find any reason for pbx_lua to export it's symbols to the rest of Asterisk, I simply changed the flag to AST_MODFLAG_DEFAULT.  Problem solved.  What I didn't realize was that the symbols need to be exported not because Asterisk needs them but because any external Lua modules like luasql.mysql need the base Lua language APIs exported (ASTERISK-17279).
> 
> Back to ASTERISK-23818...  It looks like there's an issue in pbx.c where context_merge was only merging includes, switches and ignore patterns if the context was already existing AND has extensions, or if the context was brand new.  If pbx_lua is loaded before pbx_config, the context will exist BUT pbx_lua, being implemented as a switch, will never place extensions in it, just the switch statement.  The result is that when pbx_config loads, it never merges the switch statement created by pbx_lua into the final context.
> 
> This patch sets pbx_lua's modflag back to AST_MODFLAG_GLOBAL_SYMBOLS and adds an "else if" in context_merge that catches the case where an existing context has includes, switchs or ingore patterns but no actual extensions.
> 
> For later...  "dialplan show" always shows extensions, includes, then switches but pbx_find_extension actually processes extensions, switches then includes.  Which is correct?
> 
> Also, it appears that switches and includes are always inserted at the tail of their respective lists, but the lists are searched in reverse.  The result is that the last module loaded is searched for a matching extension first.  Is this expected behavior?
> 
> This patch needs to be applied from 1.8 through trunk.
> 
> 
> Diffs
> -----
> 
>   branches/1.8/pbx/pbx_lua.c 420123 
>   branches/1.8/main/pbx.c 420123 
> 
> Diff: https://reviewboard.asterisk.org/r/3891/diff/
> 
> 
> Testing
> -------
> 
> Ran the TestSuite before and after with the same results.  461 run, 397 passed, 64 failed.
> 
> Confirmed that external Lua modules properly initialize.  I used luasql.mysql as a test.
> 
> Ran tests with various merge scenarios between pbx_config, pbx_lua and pbx_ael to make sure that where contexts overlap, includes, switches, and ingore patterns are preserved in the merge process.  Extensions are different...  pbx_config and pbx_ael share the same global dialplan so the first definition of context/exten/priority wins and subsequent are rejected.  pbx_lua maintains it's own environment so even if it happens to define the same context/exten as pbx_config or pbx_ael it'll never be called because pbx_find_extension will never search pbx_lua unless the global table is exhausted.
> 
> 
> Thanks,
> 
> George Joseph
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140806/692878ce/attachment.html>