[asterisk-dev] [Code Review] Various fixes for extenpatternmatchnew

[Mark Michelson]

> On 2009-04-17 06:39:48, Russell Bryant wrote:
> > /trunk/main/pbx.c, line 1812
> > <http://reviewboard.digium.com/r/194/diff/1/?file=3045#file3045line1812>
> >
> >     lost a space on this line

I'll fix it when I commit the changes.


> On 2009-04-17 06:39:48, Russell Bryant wrote:
> > /trunk/main/pbx.c, line 1639
> > <http://reviewboard.digium.com/r/194/diff/1/?file=3045#file3045line1639>
> >
> >     I really don't like this huge macro that is defined in the middle of a function.  It really should just be a function.  But, this isn't new code, so I guess we can change it later.

I don't like these giant macros either. It made it a PITA to try to figure out what was actually happening in this function. In my branch, I can move these out so they're separate functions and re-run my tests just to be safe.


- Mark


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/194/#review709
-----------------------------------------------------------


On 2009-03-13 15:35:46, Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/194/
> -----------------------------------------------------------
> 
> (Updated 2009-03-13 15:35:46)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> When looking into issue 14615, I found a variety of problems which I have addressed in my npm_fixes branch. Here is a summary of the fixes I made:
> 
> 1. Create different trie entries for pattern and non-pattern matches which are otherwise identical. For example, if you had an extensions "NNN" and "_NNN" in your dialplan, then we need to create two separate trie entries for matching against.
> 
> 2. Make sure to only apply pattern-matching logic if the extension we are matching against is a pattern extension. This is what bug 14615 is actually about. In that example, the reporter was attempting to Goto() the extension "ANSWER." The problem was that when the 'N' was encountered while looking at the pattern trie, it would not match the literal character 'N' that was being passed in. However, if the Goto() were modified to go to something like "A4SWER" it would go to the ANSWER extension. This behavior has been corrected.
> 
> 3. Fixed a few places where the code could potentially crash due to dereferencing NULL pointers.
> 
> 4. Changed the allocation scheme of a match_char to use the "allocation after the struct" method.
> 
> 5. Did a bunch of coding guidelines-related cleanup.
> 
> 6. Removed all blocks of code enclosed inside #ifdef NOTNOW...#endif. These areas were designed to use a more manual algorithm for looking up contexts than just finding them in a hashtab. The logic used in these blocks was off, and I can't think of any good reason why you would want to do this.
> 
> For anyone who wants to understand how the pattern-matching trie works before taking a crack at this review, you can find a large comment in main/pbx.c that Murf wrote that does an excellent job of explaining things. This comment block is located just below the function pbx_destroy().
> 
> 
> This addresses bug 14615.
>     http://bugs.digium.com/view.php?id=14615
> 
> 
> Diffs
> -----
> 
>   /trunk/main/pbx.c 182115 
> 
> Diff: http://reviewboard.digium.com/r/194/diff
> 
> 
> Testing
> -------
> 
> I tested the following dialplan, admittedly not the most thorough, but it gets the job done:
> 
> exten => 333,1,NoOp(Starting Test, exten 333)
> exten => 333,n,Goto(ANSWER,1)
> exten => 333,n,Hangup
> 
> exten => NNN,1,NoOp(Starting Test, exten NNN)
> exten => NNN,n,Playback(tt-monkeys)
> exten => NNN,n,Hangup
> 
> exten => _NNN,1,NoOp(Starting Test, exten _NNN)
> exten => _NNN,n,Playback(tt-weasels)
> exten => _NNN,n,Hangup
> 
> exten => ANSWER,1,NoOp(Test no answer)
> exten => ANSWER,n,Playback(vm-goodbye)
> exten => ANSWER,n,Hangup
> 
> exten => _1[2-9],1,NoOp(Starting Test, exten _1[2-9])
> exten => _1[2-9],n,Playback(queue-thankyou)
> exten => _1[2-9],n,Hangup
> 
> With this, I could test numeric extensions, string extensions, patterns with reserved letters (i.e. N, Z, X), and patterns with bracketed expressions. In all cases, the pattern I expected to match was matched. The reporter on issue 14615 has also given the branch a test and says that things work for him in his dialplan now.
> 
> 
> Thanks,
> 
> Mark
> 
>