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

[Alex Rousskov]

On 11/30/2016 08:12 AM, Garri Djavadyan wrote:

> I've attached fixed version.

> +        // If-None-Match did not match; treat as an unconditional hit
> +        return false;

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 apologize for using the wrong comment wording in the earlier sketch.


>      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.


> On Thu, 2016-12-01 at 01:20 +1300, Amos Jeffries wrote:
>> 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.
> 
> Should I revert TCP_INM_HIT to TCP_IMS_HIT at the moment?

I do not know the answer to that question, but it sounded like Amos was
OK with your changes regarding the INM pseudo-flag.

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.


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.


Thank you,

Alex.