Re: [PATCH] StoreID via eCAP and ICAP

On 28/06/2014 11:54 a.m., Alex Rousskov wrote:
> Hello,
>
> The attached patch allows eCAP and ICAP services to set StoreID
> values, just like many existing store_id_program helpers do. To avoid
> surprises from rogue services, the optional behavior must be enabled in
> squid.conf by adding "store-id=1" to the e/icap_service directive.
> Adaptation services are executed before the StoreID helpers but they may
> co-exist. The latest StoreID value wins.
>
> The patch also merged eCAP and ICAP code that updates adaptation
> history, resolving an old TODO.
>
> Disclaimer: After these changes, options returned by broken eCAP
> adapters that implemented adapter::Xaction::option() without also
> implementing adapter::Xaction::visitEachOption() will stop working.
> Similarly, options returned by broken eCAP adapters that implemented
> visitEachOption() without also implementing option() may start working.
> I do not think this is a big deal because the loss of backward
> compatibility is limited to broken adapters that would have to be
> rebuild for newer Squid/libecap anyway.
>

Audit:

in src/adaptation/History.h:
* store_id member is not following the camelCase member naming for this
class.

in src/cf.data.pre:
* please omit the 0|1 values from the documentation. on|off is
sufficient for new option parameters and allows non-numeric
implementation in future.

in src/client_side_request.cc:
* s/emtpy/empty/

+1. The above can be done and the patch applied without another audit
review IMO.

>
> I used "Store-ID" for the name of the meta header (an ICAP response
> header or an eCAP option name; not an HTTP header!). Other alternatives
> considered but rejected:
>
> Store-Id: Most registered MIME headers use -ID, not -Id.
> http://www.iana.org/assignments/message-headers/message-headers.xhtml
>
> X-Store-ID: Trying to avoid adding X-Store-ID now and then renaming it
> to Store-ID ten years later, while adding more code for backward
> compatibility reasons.

Use of X- prefix for experimental headers is officially frowned upon by
IETF and IANA for the same reasons. I agree with not going there.

> The Store-ID header is not registered. I am not
> going to write an Internet Draft to describe it, but somebody could try
> to submit a _provisional_ Store-ID IANA registration that does not
> require a Draft or an RFC AFAICT. However, the header would still need a
> description on the web somewhere. Perhaps somebody can volunteer to take
> a lead on that?
>
> Archived-At: This is a registered header that is almost perfect for our
> needs. The was worried that other developers would object to using a
> completely different name compared to StoreID helpers (and the feature
> name itself).

I'm not worried about the naming. The semantics fit. If you want to use
this it makes a lot of sense and saves all the work of documenting an
identical header.

Amos
Received on Tue Jul 29 2014 - 08:59:34 MDT