Re: [MERGE] c++-refactor HttpHdrCc

On Thu, Sep 29, 2011 at 4:28 PM, Alex Rousskov
<rousskov_at_measurement-factory.com> wrote:
> On 09/29/2011 12:53 AM, Kinkie wrote:
>
>>>>          case CC_MAX_STALE:
>>>> >> -
>>>> >> -            if (!p || !httpHeaderParseInt(p, &cc->max_stale)) {
>>>> >> +            if (!p || !httpHeaderParseInt(p, &max_stale) || max_stale < 0) {
>>>> >>                  debugs(65, 2, "cc: max-stale directive is valid without value");
>>>> >> -                cc->max_stale = -1;
>>>> >> +                maxStale(MAX_STALE_ANY);
>
>
>>> > The above will treat max-stale=invalid as a valid valueless max-stale. I
>>> > think it would be better to treat malformed max-stale as no max-stale at
>>> > all. How about setting MAX_STALE_ANY _only_ when p is nil?
>
>
>> Fine by me, but from what I can tell I'm preserving the current behaviour.
>
>
> Yes you are preserving the current [incorrect] behavior except, perhaps,
> for negative values.

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.

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

Thanks

-- 
    /kinkie