[squid-dev] Fix If-None-Match processing and related bug 4169

[Garri Djavadyan]

On Sun, 2016-12-04 at 20:05 -0700, Alex Rousskov wrote:
> Actually, in this case, If-None-Match matched (i.e., "no one matched"
> is
> true). And this is exactly why we can ignore that conditional header
> field and treat the request as an unconditional hit. All these
> conditional headers have the same overall logic: If the answer to the
> "If" question asked by the header field name is "yes", then the
> header
> field is essentially ignored.
> 
> How about:
> 
>   // None-Match is true (no one matched); treat as an unconditional
> hit

I agree.


> >      if (r.flags.ims) {
> >          // handle If-Modified-Since requests from the client
> >          if (e->modifiedSince(r.ims, r.imslen)) {
> > +	    // object modified; treat as an unconditional hit
> > +	    return false;
> 
> The comment sounds weird because it is not clear which object it is
> talking about and when it was modified: Client's object or Squid's
> object? Since client cached it or since Squid cached it? The answer
> is,
> of course, that the resource was modified since the client has cached
> its response (but before Squid cached its response). I suggest using
> something like this instead:
> 
>   // Modified-Since is true; treat as an unconditional hit
> 
> These comment fixes can be done during commit IMO.

I agree, it looks far better.


> If you have not done so, please do check _all_ TCP_IMS_HIT
> occurrences
> (at least) to make sure the surrounding code is still valid after
> splitting TCP_IMS_HIT into two values.

Yes, I've done it first of all.


> Disclaimer: I only sanity-checked the modified code. I did not
> validate
> whether the changes actually fix the bugs you referred to. I trust
> they
> do and do not plan to review this further.

Alex, Amos many thanks for the review.


NOTE: I'm a subscribed squid-dev user. Feel free to omit Cc header
filling. 

Garri