On 11/11/2012 6:28 p.m., Amos Jeffries wrote:
Updated patch attache, containing the updates below...
If there is no objections I will commit this to trunk soon. It has
passed 10+ days with no additional followup.
Amos
On 11/11/2012 1:37 p.m., Alex Rousskov wrote:
On 11/10/2012 04:14 AM, Amos Jeffries wrote:
I do not know where the above documentation has to go. The helpers
message format page on Squid wiki?
The wiki, cf.data.pre, and the release notes. The above weird syntaxes
are not documented so far and I would prefer to keep it that way so we
can eliminate them one day.
The officially supported format for helper response keys is ONLY:
k=foo
k="foo"
k="foo\ bar"
k="a\ \"quoted\"\ word"
k=foo%20bar
All other syntax which might be accepted by the parser is not
officially
supported and may disappear without notice.
Sounds good to me (but see below about excessive use of backslashes
inside quoted strings -- they are not needed).
Yes I know, not needed but acceptible.
The response syntax for all helpers becomes:
[channel-ID SP ] result [ SP key-pair ... ] [ SP other] EOL
+ * line := [ result ] *#( key-pair )
There is such a thing as "key:value pair", but "key-pair" does not
make
sense to me. I suggest replacing "key-pair" with "key=value" or
"note".
The latter would need to be spelled out separately, of course.
Would kv-pair, the other way of writing it be more familiar?
Yes, I think "kv-pair" would be better (and it appears to be more widely
used than "key-pair", which is usually found in "a pair of public and
private keys" context. "Key-value pair" appears to be "standard" for our
context, even with its own wikipedia entry.
Okay will do.
+ // the value may be a quoted string or a token
+ const bool urlDecode = (*p != '"'); // check before moving p.
+ char *v = strwordtok(NULL, &p);
I do not think this works 100% as intended because strwordtok() is going
to skip leading spaces and may discover a quote _after_ them:
key= "quoted value that should not be rfc1738_unescape()d\n
but will be"
I realize that spaces after "=" are not officially legal, so I cannot
insist on you fixing this. In fact, somebody might even [ab]use this bug
to shovel quoted values that they _do_ want to be rfc1738_unescape()d so
there is some value in ignoring this discrepancy.
If you do want to fix it, you may want to add an optional "bool
*wasQuoted" parameter to strwordtok() so that the caller may check
whether the parsed string was quoted. ConfigParser::ParseQuotedString()
uses that trick.
Fixed it by checking for xisspace() on the first character after '='.
We don't want to tokenize any kv-pair at all if they sent a message
like "ERR foo= is missing".
+ String key(other().content());
If you can make "key" const, please do.
Attempting.
+ void consumeWhitespace(); // removes all prefix whitespace,
moving content left
Please make the fact that we are only consuming the whitespace prefix
and not the suffix explicit in the new method name. For example, the
following names may work better:
consumeWhitespacePrefix();
Okay. using this one.
+ const char *p = buf;
+ for(; p<=end && xisspace(*p); ++p);
When p is end, we should not dereference it.
Most MemBuf methods either work with nil buf or assert that buf is not
nil. You may want to do one or the other.
Done. Made this p<end and wrapped the whole function in a check on
contentSize().
+ void consumeWhitespace(); // removes all prefix whitespace,
moving content left
Use doxygen-style comment for the new method?
Doh. Fixed.
+ All response keyword values need to be a single token with URL
+ escaping, or enclosed in double quotes (") and escaped using \ on
+ any quotes, whitespace or \ characters within the value
+ For example:
+ user="John\ Smith"
+ user="J.\ O\'Brien"
+ user=John%20Smith
Do you want to mention that "\r" and "\n" insert CR and LF instead of
actually escaping "r" and "n"?
One does not need to escape spaces inside double quoted strings. Only
double quotes must be escaped.
One does not need to escape single quotes inside double quoted strings.
strwordtok() does not treat single quotes specially.
You may want to mention that double quotes are removed from the double
quoted strings before the value is interpreted by Squid.
Updated.
Amos
