Re: [MERGE] c++-refactor HttpHdrCc

On 09/28/2011 04:43 PM, Kinkie wrote:
> On Wed, Sep 28, 2011 at 6:02 PM, Alex Rousskov wrote:
>> On 09/28/2011 07:23 AM, Kinkie wrote:

> For the next class, we may want to do more frequent and smaller
> merges, even if it means that there will be an interim phase.

Not sure I like the sound of "interim phase", but I am all for smaller
patches as long as they are self-contained. This project, for example,
has two nearly independent parts: performance optimization using
std::map for lookups and general code polishing. And you could have
split it into even smaller parts.

> What we _can_ do is make HttpHdrCc::parse explicitly require that all
> parsed headers be >= 0 . (done that, btw)

I thought httpHeaderParseInt() already guarantees non-negative values,
but I now see that it does not. You did the right thing by adding more
parsing checks in v7.

> + if (setting) {
> + if (new_value < 0)
> + debugs(65,3,HERE << "rejecting negative Cache-Control directive " << hdr
> + << " value " << new_value );
> + } else {
> + new_value=-1; //rely on the convention that "unknown" is -1
> + }
> +
> + value=new_value;
> + setMask(hdr,setting);

The "new_value < 0" check only affects debugging, but the debugging
message implies that we are _rejecting_ the negative value. AFAICT, we
still set the mask bit on, even if the value was negative. Should that
code set "setting" to false in addition to calling debugs()?

>>> + // max-stale has a special value (MAX_STALE_ALWAYS) which correspond to having
>>> + // the directive without a numeric specification, and directs to consider the object
>>> + // as always-expired.
>>
>> Your always-expired definition is the opposite of what valueless
>> max-stale means, which is "the client is willing to accept a stale
>> response of any age."
>
> My fault, it's what I had (mis)understood from the code.

That comment still needs fixing. I would do something like:

// We use MAX_STALE_ANY value to mark valueless max-stale directive
// which instructs us to treat the response of any age as fresh.

And place that comment where MAX_STALE_ANY is declared, replacing the
current non-explanation there.

>> You may want to set MAX_STALE_ANY to the maximum valid value. This
>> should not be necessary, but if there are bugs where we use max-stale
>> and do not check for MAX_STALE_ANY it will help mask some of them.
>
> Actually, I believe it would be the perfect solution also from a logic
> point of view.
> (implemented). The only reason to keep it alive is this code (from refresh.cc):
>
> if (cc->haveMaxStale() && staleness > -1) {
> if (cc->maxStale()==HttpHdrCc::MAX_STALE_ANY) {
> /* max-stale directive without a value */
> debugs(22, 3, "refreshCheck: NO: max-stale wildcard");
> return FRESH_REQUEST_MAX_STALE_ALL;
> } else if (staleness < cc->maxStale()) {
> debugs(22, 3, "refreshCheck: NO: staleness < max-stale");
> return FRESH_REQUEST_MAX_STALE_VALUE;
> }
> }
>
> If we have no real need to distinguish the FRESH_REQUEST_MAX_STALE_ALL
> and FRESH_REQUEST_MAX_STALE_VALUE this piece of code could be
> simplified.
>
> This change also can allow us to change the data type from int32_t to
> uint32_t and, together with the getters/setters, get rid of all
> UNKNOWN states (we now consitently use the bitmask to check for
> set/unset).

We would always need MAX_STALE_ANY or equivalent because we need to
output max-stale and not max-stale=.... when we got a valueless
max-stale. However, it is likely that some of the other code can indeed
be simplified.

>>> + if (flag == CC_MAX_STALE && maxStale()!=MAX_STALE_ALWAYS)
>>> + packerPrintf(p, "=%d", (int) maxStale());
>>
>> Why do we never pack MAX_STALE_ALWAYS? Either pack it or add a comment
>> explaining why it is parsed but never packed.
>
> We reproduce what has come in, I guess.

I found the code that implicitly packs MAX_STALE_ANY! Please add the
following comment near the "(flag == CC_MAX_STALE &&
maxStale()!=MAX_STALE_ANY)" packing check:

// for MAX_STALE_ANY, we already packed bare "max-stale" above

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

> I'm testing the patch. However the longer we gon on (and the larger
> the scope gets) the less I am confident that no bugs were introduced.

Same here, but we "go on" not because I want to enlarge the scope, but
because there are new bugs introduced. If there are no bugs, any
polishing can usually be done in one or two iterations.

I do not see any serious problems in v7 though (let's not count the
wrong always-expired comment as a bug). I think it can be committed with
the above polishing touches unless others with fresh eyes can find bugs.

Thank you,

Alex.
Received on Thu Sep 29 2011 - 05:05:52 MDT