Re: [MERGE] c++-refactor HttpHdrCc

On Wed, Sep 28, 2011 at 6:02 PM, Alex Rousskov
<rousskov_at_measurement-factory.com> wrote:
> On 09/28/2011 07:23 AM, Kinkie wrote:
>
>> Attached is version 6; run-tested.
>> Includes StringArea.h this time.
>
> Sorry, I found a few more bugs in this patch. They are detailed below,
> together with a more polishing touches.

Ok, Thanks for your time.
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.

>> +    // default constructor is disabled
>> +    StringArea();
>
> Remove. There is no generated default constructor for classes with other
> constructors.

Done.

> There are generated copy constructor, destructor, and assignment
> operator, but they all do what we want them to do.

Yes. That's why I didn't hide them. They're also needed by std::map.

>> +/// iterate over a table of http_header_cc_type structs
>>  http_hdr_cc_type &operator++ (http_hdr_cc_type &aHeader)
>
> This operator does not know anything about tables. It simply increments
> its argument.

Changed to "used to walk a table of http_header_cc_type structs".

>> +    inline void maxStale(int32_t newval) {
>> +        if (newval < 0 && newval != CC_MAX_STALE) return;
>
> CC_MAX_STALE is an identifier, not a special value. Did you mean
> MAX_STALE_UNKNOWN? And "||" instead of "&&"?

You're right, but it was meant to be MAX_STALE_ALWAYS (special value,
-2), so && should be ok.

>> +    inline void maxAge(int32_t newval) {if (newval < 0) return; max_age = newval; setMask(CC_MAX_AGE); }
>> +    inline void sMaxAge(int32_t newval) {if (newval < 0) return; s_maxage = newval; setMask(CC_S_MAXAGE); }
>> +    inline void minFresh(int32_t newval) {if (newval < 0) return; min_fresh = newval; setMask(CC_MIN_FRESH); }
> ...
>
> I think we should either assert/throw OR clear the mask when the value
> is negative, but we should not silently ignore it.

I don't think it would be a good idea. These are externally-supplied
values; asserting here would be a quite easily-exploitable DOS trap.
What we _can_ do is make HttpHdrCc::parse explicitly require that all
parsed headers be >= 0 . (done that, btw)
At most we could debugs() to show it to the admin.

> Also, please s/newval/newVal/ or s/newval/v/.
Done.

> I do not think you need to say "inline" when you are providing a method
> definition at the time of the method declaration.

Ok. removed.

> BTW, you may want to consider using one macro to define four standard
> methods for integer-valued directives and another macro to define four
> standard methods for boolean directives.

I'd prefer to avoid it to improve readability. There's quite enough
metaprogramming in other languages (e.g. javascript/DOM). It becomes
quickly tiring.

> You can also add a setValue() or similar method that will implement the
> above logic, given a reference to the data member and directive ID. It
> is wrong to duplicate so much code! Sorry if I missed that in my earlier
> sketch.

If I understand what you mean correctly, it doesn't really buy much
(and it'll have to be duplicated for every class with similar
semantics).
I've implemented it anyways, I hope it fits your idea.

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

> s/MAX_STALE_ALWAYS/MAX_STALE_UNLIMITED/ or
> s/MAX_STALE_ALWAYS/MAX_STALE_ANY/ (I will assume the latter in the
> comments below).
>
> 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).

> Add a boolean maxStaleAny() to avoid exposing the caller to your special
> MAX_STALE_ANY constant.
> You could also s/maxStale()/maxStaleFixed()/ to emphasize that the
> caller should not trust maxStale() values blindly. Your call.

See above.

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

>> +            int32_t ma;
>> +            if (!p || !httpHeaderParseInt(p, &ma)) {
>
> You did not add local variables for parsing other directives. Do not
> make max-age special.

The others are actually broken, they do not set the mask.

> However, note that httpHeaderParseInt() needs an int, and not int32_t
> parameter. This is one of the reasons why you probably should not change
> the type of these values.

If needed I'll change them back; I dislike the level of uncertainty
"int" leaves in, so I'd just use a temp unless the compilers can
automatically adjust.

>> -        } else {
>> -            EBIT_SET(cc->mask, type);
>>          }
> ...
>> +            if (!p || !httpHeaderParseInt(p, &s_maxage)) {
>>                  debugs(65, 2, "cc: invalid s-maxage specs near '" << item << "'");
>> +                clearSMaxAge();
>>              }
>>              break;
>
> AFAICT, we are now missing a call to set the bit mask for s-maxage. Same
> for many other directives! If I am right about this, please note that
> your testing was pretty weak since it did not detect these bugs.

I tested with a live browser and checked that stats were reported. They were.

> Finally, looking at the actual usage, I would s/haveFoo/hasFoo/ in the
> new method names because "cache_control" is singular. "Has" is also
> shorter :-).

Done.
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.
Please find v7 attached.

Thanks

-- 
    /kinkie