On 23/02/2013 10:20 a.m., Alex Rousskov wrote:
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.
Hmm. Right and these two are different from the other options in that
the others are singleton options. Will do.
+ } 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 the no-cache cases the recovery action is not critical like private
might be. All we risk with this is a cache HIT vs REFRESH.
Still I'm not partial either way. Will do.
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.
All these permutations have been worrying me a little.
// 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.
Okay.
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.
Yes I know.
Amos
