On 27/09/2013 6:14 a.m., Alex Rousskov wrote:
On 08/30/2013 11:39 AM, Alex Rousskov wrote:
One of the key ideas behind "strict syntax" is that the token "type" can
be and is determined by the parser (simple token, quoted string, regular
expression, number, etc.) and not the caller. This is not the only way
to support better syntax rules, but it is overall simpler and more
flexible than the caller-determines-token-type alternatives (because the
code can manipulate tokens with respect to comments, macros, line
continuation, whitespace, and such without relying on callers to parse
each character).
Hello,
While I still believe that the above is correct, after many email
exchanges with Christos and several patches, I suspect we should not
implement the above. My concern is that if we do, the fixed syntax will
never become the default because there will be just too much work for
admins to rewrite their configurations, parts of which they may not even
control. Initially, I thought we can remain mostly backwards compatible
with the fixed syntax, but I think I lost that important goal along the
way to the strict syntax.
To void any misunderstanding, these are my thoughts. Christos has
supplied valuable insight and very useful code that brought me here, but
he has not reviewed and may not share my conclusions.
Hopefully, this discussion will help us find the right balance between
the needed syntax improvements and the need to remain backwards
compatible in the vast majority of cases.
I think the key problem here is our (or my) desire to use a simple
grammar definition for the strict syntax. I think we have to relax that
by using Squid context-aware parsers more aggressively. Also, we have to
make a couple of new syntax constructs less beautiful to minimize
collision chances. Initial deployment of v3.4 code exposed a few
unexpected (but unfortunately common) collisions.
Specifically, I propose the following:
A) Define Squid syntax as described in the grammar quoted at the end of
this email. Some adjustments would be necessary, I am sure, but I think
it is important to have a grammar definition as a starting point. They
key in that grammar is the ambiguous rule after "The caller must ask"
comment. If it works, it will allow us to support most of the old
configurations, especially when dealing with old options.
B) Adjust the low-level parsing code to provide the following methods,
each of which extract the requested thing and return true (on success)
or false (on failures). If they fail to extract what was requested, they
leave the parsing stream unchanged:
NextChars(const char *); // the exact sequence of characters given
NextToken(String &t); // next name or token as defined in the grammar
NextValue(String &v, kind); // next value as defined in the grammar
NextSpace(String *v); // Optional White Space (OWS) as in grammar
NextLineEnd(); // end of line; may be \0 if preprocessor strips \n
Mark CurrentPosition(); // current parsing position
void RevertTo(Mark mark); // reverts low-level parsing state to mark
Also add the following class to properly manage "undo". Note that we
cannot just put parsed elements on stack any more because the next
caller may use a different syntax to parse the same sequence of characters.
This implies that we require StringNG terminator-agnostic ability for
the String& in all the above functions - OR to copy each potential line
into a temporary String which can be dropped on unwind. That
terminator-agnostic is provided by StringArea, but IIRC kinkie warned
against relying on StringArea for parser usage due to its lack of cow()
and pointer safety.
// remembers CurrentPosition() upon construction and undoes damages
// using RevertTo upon destruction unless .success() has been called
class ParseProgress {
public:
ParseProgress(): start(CurrentPosition()), succeeded(false) {}
~ParseProgress() { if (!succeeded) RevertTo(start); }
bool success() { return succeeded = true; }
bool failure() const { return false; } // for clarity
private:
Mark start; ///< where the parsing started
bool succeeded; ///< whether our owner successfully parsed
};
C) Add parameter parsing helpers sketched below, using the above API:
bool ParamWithValue(const char *name, String &value, bool required)
ParseProgress progress;
if (!NextChars(name))
return progress.success();
if (!NextChars("=")) {
if (required) ... failure: missing value for name
return false;
}
// we may need to make strictOrLegacy a fourth ParamWithValue()
// parameter so that callers can control what kind of values
// are allowed
if (NextValue(value, strictOrLegacy))
return progress.success();
if (required)
failure: missing value for name
return false;
}
// directive parameter without a value
bool ParamWoutValue(const char *name)
{
if (!NextChars(name))
return false;
if (NextChars("="))
failure: unexpected value for name
return true;
}
bool MoreParams()
{
return NextSpace();
// the line end will be checked for and absorbed
// by the directive parser, not directive parameters parser
}
// called when all supported switches and parameter names
// have been exhausted; we do not expect a name=value thing next
// kills Squid on errors; otherwise returns false
bool UnsupportedParamName()
{
ParseProgress progress;
String name;
if (NextToken(name) && NextChars("="))
failure: unknown directive parameter name or switch: name
return progress.failure(); // undo name extraction
}
Here is how a simple directive parameters parser would look like:
do {
String v;
if (ParamWithValue("opt1", v, true)) ...
else if (ParamWithValue("opt2", v, false)) ...
else if (ParamWoutValue("opt3")) ...
else if (!UnsupportedParamName()) {
NextValue(v, ...) // anonymous parameter
...
}
} while (MoreParams());
The above can be rearranged so that the directive parameters parsing
code first extracts the parameter name and then compares it to supported
parameters (like most of the current code does). However, I suspect it
would create more problems than it would solve in some cases: The
extracted thing may not be a parameter name but an [anonymous] value,
and when you extract a value, you may want to tell the parser whether
using relaxed syntax is allowed, but you cannot tell the parser that
until you are sure that the next thing is not a parameter name (because
the relaxed value parser is going to eat the '=' sign)! Thus, you should
not extract a value until you know it is not a parameter name.
Agreed +1. This is no less efficient than the current code doing its
series of if-conditions on a strtok() output string. But gains us the
undo functionality a lot more cheaply.
I am omitting some details, of course, but do you think this approach
with more explicit whitespace and relaxed value handling will cover
nearly all current use cases(*), allowing the new parsing mode to be
enabled by default soon? Or am I missing something again?
Comment handling (see blow for the fix), and the detail that StringNG
will allow us to implement the undo fuctionality far better than String
- which implies that the large amount of conversion work will have an
almost as larhe amount of conversion to SBuf later in 3.5.
Since this design is again prioritizing backward compatibility I think
we can schedule this for 3.5+ and work on some bridging polish to avoid
3.4 parser having any user noticible difference from the final strict
parser. For starters ignore the code internals being vastly different
maintenance nightmare - that is not a user-visible aspect and we will
cope with that pain ourselves.
What I am thinking is that if we agree on this grammar being suitable
(you and I mostly agree, lets see what the others think) then we can
massage the 3.4 parser to cope with it. No matter how nasty the hacks
may have to be in that version we drop them in 3.5 going forward. I do
not see re-implementing the entire parser starting today as a quick
enough project to fix things before 3.4 needs to be stable.
(*) except quoted strings with unsupported %macros in old configs, of
course, but we are willing to sacrifice those, I guess. The syntax also
allows comments where spaces are allowed, but we can disable that if
there are too many clashes.
If we take a token of "#" surrounded by whitespace as comment start, or
a # in the position EOL is expected this should not be an issue for most.
Since comments go to EOL it will collide with the preprocessor
un-wrapping wrapped lines if a comment is added anywhere space is
possible. I am inclined to make comment a case in itself with the
following alterations to the grammar:
config = *( OWS directive / comment <new line> )
directive = name *( RWS parameter ) comment <new line>
whitespace = *ascii_whitespace_char
comment = OWS ( <#> RWS <followed by all characters until the end of the
line (excluding that EOL)>)
Line wrapping grammar is this:
fold = <\> <new line>
whitespace = *( ascii_whitespace_char / fold )
with no need or possibility to fold inside a comment.
Thank you,
Alex.
Squid configuration syntax:
config = *( OWS directive OWS )
directive = name *( RWS parameter ) OWS <new line>
name = token
parameter = anonymous_parameter / named_parameter
anonymous_parameter = value
named_parameter = name <=> value ; No spaces around = allowed!
; This is where the grammar is ambiguous!
; The caller must ask for either strict_value, relaxed_value, or
; "strict_value if possible and relaxed_value otherwise"
value = strict_value / relaxed_value
strict_value = token / percent / RE / fcall /
single_quoted_string / double_quoted_string
; any non-whitespace sequence that does not start with one of
; the two reserved prefixes: "squid::" and "regex::"
relaxed_value = xchar *xchar ; with prefix restrictions
; The "squid::" prefix avoids fcall clashes with relaxed_values
; and allows us to add more functions later.
; We may require that fcall is the only or last directive parameter.
fcall = "squid::" name <(> OWS farguments OWS <)>
; we can support just one required argument for now
farguments = fargument *( OWS <,> OWS <fargument> )
fargument = strict_value
; TBD, but reserve "regex::" now to avoid clashes with relaxed_values
RE = seven characters "regex::" followed by a self-delimiting
sequence of characters
token = tchar *tchar
percent = alphanumeric *alphanumeric <%> ; add <.>?
single_quoted_string := <'> *[sqchar / escaped-pair] <'>
double_quoted_string := <"> *[dqchar / escaped-pair] <">
tchar = alphanumeric / <_> / <-> / <.>
xchar = any char except ascii_whitespace_char and new line
sqchar = any char except single quote and backslash
dqchar = any char except double quote and backslash
escaped-pair = backslash followed by any char except new line
OWS = *[whitespace] ; optional whitespace
RWS = whitespace OWS ; required whitespace
whitespace = *ascii_whitespace_char / comment
comment = <#> followed by all characters until the end of the line
comments should be a # _after_ some whitespace. Not instead of,
otherwise we cannot tell foo#bar from "foo #bar"
Please note that the above grammar is applied after the preprocessing
stage which handles lines continuations using backslashes, conditionals
and ${macros}. No changes there. Unfortunately, our preprocessor cannot
tokenize because tokenization should remain context-dependent for
backward compatibility reasons.
Thank you. It is good to have this written down finally.
Amos
