Re: [PATCH] HelperReply upgrade stage 2

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.

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

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

> + char *v = strwordtok(NULL, &p);
...
> + safe_free(v);

strwordtok() does not allocate v. Why are we freeing it?

HTH,

Alex.
Received on Fri Nov 09 2012 - 16:21:49 MST