[asterisk-dev] [Code Review] 3161: res_sorcery_astdb.c: Fix regex handling and keep simple prefix matching performance.

[rmudgett]

> On Jan. 28, 2014, 4 a.m., wdoekes wrote:
> > /trunk/res/res_sorcery_astdb.c, lines 277-281
> > <https://reviewboard.asterisk.org/r/3161/diff/1/?file=53171#file53171line277>
> >
> >     So, I deduce that:
> >     
> >       tree=="" means tree=="%", but
> >       tree="abc" does not mean tree="abc%"
> >     
> >     ?
> >     
> >     (And if we have ^ and no regex at all, we could skip the regcomp and regexec. Right?)

Those deductions are correct.

We could skip the regcomp() and regexec() if regex is "^" but the extra complexity doesn't seem worth it.


> On Jan. 28, 2014, 4 a.m., wdoekes wrote:
> > /trunk/res/res_sorcery_astdb.c, lines 229-231
> > <https://reviewboard.asterisk.org/r/3161/diff/1/?file=53171#file53171line229>
> >
> >     I'd rather see this in a separate function. It takes up half the space in this one right now.

Extracted.


> On Jan. 28, 2014, 4 a.m., wdoekes wrote:
> > /trunk/res/res_sorcery_astdb.c, lines 247-250
> > <https://reviewboard.asterisk.org/r/3161/diff/1/?file=53171#file53171line247>
> >
> >     And no need to add % below, right?

Right


> On Jan. 28, 2014, 4 a.m., wdoekes wrote:
> > /trunk/res/res_sorcery_astdb.c, lines 269-270
> > <https://reviewboard.asterisk.org/r/3161/diff/1/?file=53171#file53171line269>
> >
> >     Adding twice? (You don't even have enough space.)

There was enough room in the tree[] declaration.  (The '^' is removed and two '%' added plus a nul terminator.)  However, I did fail to recognize that the "%%" in the printf format really means only one '%'.


> On Jan. 28, 2014, 4 a.m., wdoekes wrote:
> > /trunk/res/res_sorcery_astdb.c, lines 255-257
> > <https://reviewboard.asterisk.org/r/3161/diff/1/?file=53171#file53171line255>
> >
> >     Your comments are right. Fix the nextchar check and add a quick check to see if any "|" is found (ignoring parentheses and backslashes for the sake of simplicity).
> >     
> >     Sounds like not too much work.

I originally wrote most of what the comment describes but then realized that I didn't know enough about regex syntax to be sure of my logic.  Also I don't think we really want to make this code that smart about regex syntax.  The simple prefix case of "^abc" is the driving force of this patch so CLI tab completion and PJSIP REGISTER contact processing won't use so much memory and CPU.


- rmudgett


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


On Jan. 27, 2014, 4:06 p.m., rmudgett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3161/
> -----------------------------------------------------------
> 
> (Updated Jan. 27, 2014, 4:06 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> The sorcery astDB wizzard does not handle regex correctly if the pattern begins with an anchor character.
> 
> This patch attempts to convert the anchored regex pattern to a prefix pattern supported by astDB for performance reasons.  If it is not able to convert the pattern it gets all astDB members of the family and then does a normal regex pattern matching on the resulting records retrieved.
> 
> 
> Diffs
> -----
> 
>   /trunk/res/res_sorcery_astdb.c 406695 
> 
> Diff: https://reviewboard.asterisk.org/r/3161/diff/
> 
> 
> Testing
> -------
> 
> Simple regex patterns that are just prefixes are converted and the astDB just fetches the matching records.  More complex patterns fallback to getting all astDB family records and searching them by regex.
> 
> 
> Thanks,
> 
> rmudgett
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140218/e99a561b/attachment-0001.html>