Re: [PATCH] HelperReply upgrade stage 2

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
Received on Fri Nov 23 2012 - 05:28:59 MST