On 1/12/2016 12:44 a.m., Garri Djavadyan wrote: > On Tue, 2016-11-29 at 15:42 -0700, Alex Rousskov wrote: >> On 11/29/2016 02:23 PM, Amos Jeffries wrote: >>> >>> On 30/11/2016 1:47 a.m., Garri Djavadyan wrote: >>>> >>>> On Tue, 2016-11-29 at 14:51 +0500, Garri Djavadyan wrote: >>>>> >>>>> Hello, >>>>> >>>>> Please review the attached patch prepared for r14958, it fixes >>>>> the >>>>> If- >>>>> None-Match processing (incorrect logging [1]) and the bug [2] >>>>> report >>>>> 4169 depending on the incorrect (IMO) behavior. >>>>> >>>>> An If-None-Match request for a non-matched (ETag) but cached >>>>> object >>>>> should be processed as normal HIT. >>>>> >>>>> [1] http://lists.squid-cache.org/pipermail/squid-users/2016-Nov >>>>> ember/013483.html >>>>> [2] http://bugs.squid-cache.org/show_bug.cgi?id=4169 >> >>> >>>> >>>> In the first version I >>>> removed strings (not related to the fix) which mark a request as >>>> non- >>>> IMS, because I thought the same request could not >>>> enter processConditional twice and the removed actions are >>>> redundant. >> >> I agree that the second version is better: Unless you are absolutely >> sure, it is best to stay focused on the problem and leave the IMS >> deletion code "as is" even if it may not be necessary. If you still >> suspect it is not necessary, add a TODO comment to investigate >> separately. >> >> >>> >>> Checking this whole processConditional() sequence for compliance >>> with >>> RFC 7232 and 7234 the rest of the logic seems to be fine except >>> this case: >>> >>> <https://tools.ietf.org/html/rfc7234#section-4.3.2> >>> " >>> If the field-value is "*", or if the >>> field-value is a list of entity-tags and at least one of them >>> matches >>> the entity-tag of the selected stored response, >> >>> >>> [ e->hasIfNoneMatchEtag(r) == false --> !N == true ] >> >> I am not sure I am interpreting your [...] mnemonic correctly, but >> AFAICT, the patched code works when the RFC conditions quoted above >> are >> _not_ meant: >>
Arg. You are right. Double not-non-negations strikes again :-( <snip> > > Alex, Amos thanks for comments! I've attached third version of the > patch. It is based on the proposal by Alex. The patch introduces > following changes: > > * Matched If-None-Match requests are logged as TCP_INM_HIT (proposed by > Amos) > * If-Modified-Since requests for modified cached objects are processed > as normal HITs > * Not matched If-None-Match requests for cached objects are processed > as normal HITs > * If-Modified-Since header is ignored if If-None-Match header exists > (RFC7232 compliance) > > I've tested different conditional requests (including 'If-None-Match: > *' and 'If-None-Match: "matched-tag", "not-matched-tag"') and found no > problems. Thanks. > +1. That looks better to me. I would really like the LogTags flag to be an actual flag, but I can do that as a followup or later when I convert the others to flags. Amos _______________________________________________ squid-dev mailing list [email protected] http://lists.squid-cache.org/listinfo/squid-dev
