Review Board 1.7.16


Various fixes for extenpatternmatchnew

Review Request #194 - Created March 13, 2009 and submitted

Mark Michelson
/trunk
14615
Reviewers
asterisk-dev
Asterisk
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().
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.
Ship it!
Posted (April 17, 2009, 1:39 a.m.)

   

  
/trunk/main/pbx.c (Diff revision 1)
 
 
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.
  1. 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.
  2. Actually, I'll just wait and do this at a separate time, like you suggested.
/trunk/main/pbx.c (Diff revision 1)
 
 
Same comment goes for this macro.  It's a huge macro defined in the middle of a function that really should just be a function.

If you feel like making a function out of it, that would make me happy, but it's not required since it's not new code.
/trunk/main/pbx.c (Diff revision 1)
 
 
lost a space on this line
  1. I'll fix it when I commit the changes.

https://reviewboard.asterisk.org/ runs on a server provided by Digium, Inc. and uses bandwidth donated to the open source Asterisk community by API Digital Communications in Huntsville, AL USA.
Please report problems with this site to asteriskteam@digium.com.