Re: [PATCH] HelperReply upgrade stage 2

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).

>>
>>> 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.

> + // 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.

> + String key(other().content());

If you can make "key" const, please do.

> + 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();
    trimLeft();

> + 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.

> + void consumeWhitespace(); // removes all prefix whitespace, moving content left

Use doxygen-style comment for the new method?

> + 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.

Thank you,

Alex.
Received on Sun Nov 11 2012 - 00:37:52 MST