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

[Mark Michelson]

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

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