Re: [PATCH] HelperReply upgrade stage 2

Updated patch attache, containing the updates below...

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