Re: [PATCH] HelperReply upgrade stage 2

Updated patch attached.

This one also:
* adds squid.conf documentation for changes in this update.

* adds deprecation of several external_acl_type now useless options
related to helper protocol quoting.

* adds missing dump of the protocol=2.5 option on config display.

On 10/11/2012 5:21 a.m., Alex Rousskov wrote:
> On 11/09/2012 04:28 AM, Amos Jeffries wrote:
>> This stage of the helper reply protocol upgrade adds key-pair support to
>> the HelperReply object.
>>
>> It uses the new Notes objects to hold any "key=value" syntax fields
>> received from the helper after the result code field. The values section
>> for these key-pair may be token or quoted string. Helpers which
>> previously rfc1738 encoded the values may still do so but it is no
>> longer mandatory.
> We need to be more accurate/strict here: Helper writers must know
> whether their annotation values will be rfc1738 decoded. AFAICT, the
> patch implies the following rules:
>
> * All values are parsed using strwordtok().
> * All parsed values longer than 2 bytes are rfc1738 decoded.

Okay. URL-decode is now skipped when the value starts with ", is
otherwise always run after shell-escaping is removed from the token.

>
> When the above is converted into for-admin documentation, we would need
> to replace strwordtok() function name with its description/meaning, of
> course. This will be difficult because strwordtok() is a mess! For
> example, strwordtok() allows the following annotation values:
>
> "foo "bar
> foo" "bar
> "foobar""
> foobar"
> foobar\
>
> I am guessing you have to keep using strwordtok() for backward
> compatibility reasons. You may want to consider restricting value syntax
> in Squid documentation though. We may continue to accept weird values
> above, but the documentation will prohibit them so eventually we might
> be able to make the parser more strict as well.
>
> 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.

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

>
>> Any other keys MAY be sent on any response. However at this stage 2
>> patch they are ignored. As are repeated / secondary values for the
>> expected key names, only the first instance sent in the response is used.
> Sounds good to me.
>
>
>> + char *v = strwordtok(NULL, &p);
>> + if ((p-v) > 2) // 1-octet %-escaped requires 3 bytes
>> + rfc1738_unescape(v);
> AFICT, strwordtok() may return NULL. We should test for that before we
> subtract v.

Thank you. Fixed.

>
>> + char *v = strwordtok(NULL, &p);
> ...
>> + safe_free(v);
> strwordtok() does not allocate v. Why are we freeing it?

Removed. Dross from an earlier experiment with another tokenizer before
caving on the loss of const and using strwordtok().

Amos