[Home]

Summary:ASTERISK-01284: [patch] ast_expr can't handle quoted strings, bad error messages, multiple spaces...
Reporter:Steve Murphy (murf)Labels:
Date Opened:2004-03-24 15:31:46.000-0600Date Closed:2008-01-15 14:53:09.000-0600
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Core/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) ast_expr.y.diffu
( 1) ast_expr.y.diffu2
Description:ast_expr can't handle strings with spaces, which pretty
much rules out using ${CALLERIDNAME} in it, or any other
variable that MIGHT contain a value with a space.

Parse errors give no clue at all as to what was wrong.

An extra space in the wrong place and you get a parse error.
Not enough spaces, and it will either just plain not work,
or yield a parse error.

I've made a few small changes to look for quotes as well as
spaces, and to end the token with the closing double quote.
Quotes are removed from the token.

Now, you can rest assured that this kind of construct
will parse successfully:


exten => s,6,GotoIf($[  ${CALLERIDNUM}   :   "3077545675"   &   "${CALLERIDNAME}"   :   "Privacy Manager"  ]?telemarket|s|1:s|7)

In the above, note that the stuff in $[] includes leading
multiple spaces, trailing multiple spaces, quotes, and
it works as expected, without parse errors.

To improve parse error messages, I've added two static
variables, ast_expr_argument_position, and ast_expr_argument, which keep track of what is being
parsed, and where in the string we are parsing. If a
parse error occurs, a message is generated, the
input string is output, and a series of spaces, with
an up-arrow character indicating the column in which
the parser was scanning, is output in the next line.
Thus, the end user sees any substitutions, and most likely
which token might have created the problem.

I've tested the positive branch, the negative branch,
actual parse errors, etc. Looks good.

Comments:By: Steve Murphy (murf) 2004-03-26 18:24:49.000-0600

OK, I've just added a new version of the patch.

The fact that I was using globals in a threaded environment to
go to the yyerror routine really bothered me. So I did the
homework, and played around with bison, and managed to use the
right combo of things to pass the lex data to the yyerror routine
to generate a thread-safe, useful error message.

Now, the yyerror generates two lines following the input it got.
The first line reflects data it gets from the location tracking data
that bison can generate. Basically, it seems to indicate how much
the parser has already parsed that was OK.

The second line has an arrow at the position where the lookahead
token ended. The error would most likely be between the last arrow on
the first line, and the arrow on the second line. Usually this will
make it pretty obvious, where the trouble lies.
By: twisted (twisted) 2004-04-29 09:31:08

I believe some testing is in order here.. I can see where this woudl be useful..
By: Mark Spencer (markster) 2004-05-01 21:46:26

Testing would be nice, since i'm totally lost about lex...  I made a "B" in compilers :(
By: Steve Murphy (murf) 2004-05-01 23:50:15

Suggestion: IF there is any problems, the area to study is backwards
compatibility. The features added will have these kinds of effects:

1. multiple spaces OK.  This won't affect anyone with already written
  expressions, because they either chopped the extra spaces, or their
  expr never parsed. The guys who never fixed the broken exprs may
  suddenly find that they now work. This could be highly inconvenient,
  if they were depending on it NOT working...!

2. Things in double quotes now eval to a single token. This is only
  a factor if the string in double quotes contained spaces, and that
  was GOOD. This could be the case if perhaps the text evaluates to
  legal expr syntax. This would indeed be a fairly advanced usage of
  the exprs in Asterisk. I'd personally be interested in what they
  were doing! At any rate, it seems such could be fixed by tweeking
  the input strings.

3. Better syntax error messages. Well, no-one should suffer here, only
  if it crashes... I've run the code thru various tests here, and it
  seems to work as advertised.

I'm going to delete the first version of this patch, so it isn't tempting
anyone to use it.
By: Mark Spencer (markster) 2004-05-01 23:54:19

Sure, what the heck, i'll put it in.  I assume you have a disclaimer on file?
By: Steve Murphy (murf) 2004-05-01 23:57:37

Pretty sure I sent one in. If not, I'll be happy to file another.

Oh, by the way, I guess I can't delete an attached file. Thought under
the "advanced" stuff, I'd be able to... sorry.
By: Mark Spencer (markster) 2004-05-02 00:02:36

I'll take your word for it on the disclaimer :)  Thank you very much for this generous contribution and if all heck breaks loose, we'll open this back up.
By: Digium Subversion (svnbot) 2008-01-15 14:53:09.000-0600

Repository: asterisk
Revision: 2855

U   trunk/ast_expr.y

------------------------------------------------------------------------
r2855 | markster | 2008-01-15 14:53:08 -0600 (Tue, 15 Jan 2008) | 2 lines

Merge murf's generous expression contributions (bug ASTERISK-1284)

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=2855