On Thu, Sep 29, 2011 at 5:55 PM, Alex Rousskov <[email protected]> wrote: > On 09/29/2011 09:10 AM, Kinkie wrote: > >> How about proceeding like this? >> This has already grown way past a "playground" or "refactoring" job >> (it changes some behaviour). >> We merge as-is, and then change the behaviour in a follow-up commit to trunk. > > Sounds good to me. > > >>>> Also, wouldn't that make the parser unnecessarily strict? >>> >>> Not sure what you mean. We got a malformed max-stale value. We have only >>> two options when it comes to that value interpretation, I think: >>> >>> A1. Treat it as valueless max-stale. >>> A2. Ignore it completely. >>> >>> Since valueless max-stale results in possibly very stale content being >>> served to an unsuspecting client, I think #2 is much safer (and >>> compliant with HTTP). >> >> You know way more than I do about this, so ok. >> >>> BTW, we have three options when it comes to forwarding the malformed >>> directive: >>> >>> B1. Forward our own valid directive. >>> B2. Forward nothing. >>> B3. Forward the malformed directive. >>> >>> While option B3 could be the best, it requires more parsing changes. >>> Option B2 is much better than B1, IMO, and requires minimal changes. >>> >>> We currently implement A1 and B1. It is OK to treat this problem as >>> outside your patch scope. I just hope the above summary will help >>> somebody fix this later. >> >> Implementing B3 would be trivial, if we are allowed to just forward Cc >> directives as we received them (I didn't dare do it due to my lack of >> in-depth HTTP compliance knowledge). >> >> I will gladly do it as a followup patch as the other fix above. >> Do you agree? > > Sure, but I doubt correct B3 implementation would be trivial. > > It would be indeed relatively easy to forward a malformed max-stale > directive "as is" using the "other" member, but if Squid then sets its > own max-stale, then we would be forwarding two max-stales, which is not > kosher. Similarly, if we gain some code to filter directives (e.g., > using squid.conf ACLs), this simple B3 implementation would not filter > malformed ones. > > It would be OK to ignore all these problems if we could somehow prevent > new/future Squid code from bumping into them accidentally, but I cannot > think of a neat way to do that. > > IMO, we should not attempt B3 and stick with B2 for now. B2 is trivial.
Ok. If that's fine by you then I'll push ahead the merge of the currently-implemented feature set to this evening, and start working on A2 (B2 is a natural consequence of it). -- /kinkie
