Re: [PATCH] support parameters for no-cache and private

On 02/21/2013 06:26 PM, Amos Jeffries wrote:

> Adjusted patch to drop the odd NP, rework CC:private operation on broken
> parameters, and fix the segfault.

Hi Amos,

    Thank you for addressing most of my concerns.

> + } else if (/* p &&*/ !httpHeaderParseQuotedString(p, (ilen-nlen-1), &private_)) {
> + debugs(65, 2, "cc: invalid private= specs near '" << item << "'");
> + clearPrivate();
> + }

This still does not work correctly when there are multiple valid
CC:private=".." header fields in a message header because the
httpHeaderParseQuotedString() clears the previous private_ contents, right?

You can fix this by parsing into a local String variable and then
appending that variable contents to the previously stored headers using
the now-fixed Private() method. This approach will also eliminate the
need to call clearPrivate() on failures and then immediately undo that
with setMask(true,true).

Same concern for no-cache="..." parsing.

> + } else {
> + debugs(65, 2, "cc: invalid no-cache= specs near '" << item << "'");
> + clearNoCache();
> + }

I still do not think we should ignore the whole CC:no-cache just because
we cannot parse something, especially when there was a perfectly valid
parameterless CC:no-cache before we failed to parse something. Instead,
we should conservatively treat the whole thing as CC:no-cache.

In other words,

  Cache-control: no-cache
  Foo: bar
  Cache-control: no-cache="foo

should be treated as

  Cache-control: no-cache
  Foo: bar

rather than

  Foo: bar

You obviously disagree. Hopefully somebody will break this tie.

> // NP: request CC:no-cache only means cache READ is forbidden. STORE is permitted.
> + if (rep->cache_control && rep->cache_control->hasNoCache() && rep->cache_control->noCache().defined()) {
> + /* TODO: we are allowed to cache when no-cache= has parameters.
> + * Provided we strip away any of the listed headers unless they are revalidated
> + * successfully (ie, must revalidate AND these headers are prohibited on stale replies).
> + * That is a bit tricky for squid right now so we avoid caching entirely.
> + */
> + debugs(22, 3, HERE << "NO because server reply Cache-Control:no-cache has parameters");
> + return 0;
> + }
> +
> // NP: request CC:private is undefined. We ignore.
> // NP: other request CC flags are limiters on HIT/MISS. We don't care about here.

Please move the new code below all three NPs. As rendered now, the first
NP seems to apply to the if-statement, which confused me a lot. It is
also good to keep request CC code/comments together so that "other
request CC flags" comment makes more sense.

On a separate note, the code became more confusing (IMO) because patched
Squid now handles CC:no-cache in two places: Here, we handle the
parameter case. The caller handles both parameter and parameterless
cases by setting ENTRY_REVALIDATE. Fixing that is difficult, but we
should at least clarify what is going on. I suggest using the following
comment instead of the above TODO:

/* We are allowed to store CC:no-cache. When CC:no-cache has no
parameters, we must revalidate. The caller does that by setting
ENTRY_REVALIDATE. TODO: When CC:no-cache has parameters, strip away the
listed headers instead of revalidating (or when revalidation fails). For
now, avoid caching entirely because we do not strip on failed
revalidations. */

This will point to the other critical piece of code AND explain why
parameter case is treated "worse" than the parameterless one despite
being "better" from protocol intent point of view.

If my "because we do not strip on failed revalidations" explanation is
not correct, please let me know.

>> Since we now support HTTP/1.1 storage and revalidation of
>> Cache-Control:no-cache it is important that we at least detect the
>> cases where no-cache= and private= contain parameters and handle them
>> properly whenever possible.
>>
>> AFAIK these are still rare occurances due to the historic lack of
>> support. So for now Squid just detects and exempts these responses
>> from the caching performed. The basic framework for adding future
>> support of these HTTP/1.1 features is made available
>>
>
> Please run this past Co-Advisor to confirm the private="..." and
> no-cache="..." cases are now all "Precondition Failed".

Confirmed. Please note that CC:private="..." cases were failing
precondition before the patch as well. Apparently, somebody fixed
CC:private handling some time after r12394 and before your patch.

HTH,

Alex.
Received on Fri Feb 22 2013 - 21:20:49 MST