[squid-dev] [PATCH] Coverity fixes, part 3

[Kinkie]

On Sun, Jul 26, 2015 at 10:23 PM, Amos Jeffries <squid3 at treenet.co.nz>
wrote:

> On 27/07/2015 4:48 a.m., Kinkie wrote:
> > HI,
> >   the low-hanging fruits from Coverity's analysis have been picked, now
> > working on somewhat more complex fixes.
> > The attached patch takes a hint from two benign coverity defects to:
>
> > - refactor Digest auth's field lookup table to use a std::map instead of
> > abusing httpHeaderFieldsInfo; this improves readability and size of the
> > code, and has the added small bonus of lowering the lookup cost from
> linear
> > to logarithmic
> >
> > the Digest code has been run-tested. I'd like feedback on its style, as
> > httpHeaderFieldsInfo is abused similarly elswehere and I'm considering to
> > apply it elsewhere as well; it can then be further refined to get O(1)
> via
> > a carefully-chosen hash (via std::unsorted_map)
> >
>
> My thoughts: (sorry if its a bit rambling)
>
> I dont like spawning lots of new classes for basic things. I know its
> essentially the C++ way. But applying pattern theory can go too far
> sometimes. And this patterns usage is one case where I can see exactly
> that happening.
>
> You have a struct, a class, an enum and array. Thats a lot of custom
> infrastructure just to represent a simple set of name:id. We could as
> easily have std::map<const char*, enum> holding that directly and avoid
> all the local types except enum.
>
> There are already 5 headers currently in Squid that need these same
> operations applied, and at least as many more that should but dont even
> use the current HttpHeaderFieldAttrs related types yet.
>
>
>
> struct HttpDigestFieldAttrs should be a template class so we dont have
> to re-implement a new little struct for each enum.
>
> BUT, notice that generalizing that same structure is also where the enum
> casting hack for HttpHeaderFieldAttrs comes from in the first place.
>  The probkem was template style breaks with C++03 compilers thinking all
> enums are of "int" type. Enter lots of repeated-instantiation complaints
> from dumb compilers.
>  I've not tried it with current C++11 compilers which are quite a bit
> smarter. It may work now (or not).
>
> If *that* can be solved then refactoring HttpHeaderFieldAttrs to a
> template is better way forward. Maybe followed by replacing or
> refactoring the HttpHeaderFieldInfo bits to avoid the performance
> problem you identified.
>

Hi,
  no rambling, it's ok.
Actually, the performance improvement is just a nice byproduct, that's not
the objective at all, as I tried explaining in the merge proposal.
The actual problem I was addressing is the multiple C casts used to try and
coerce anything resembling a map<const char *, int> into an http/1 header
table, simply because there is code in squid which implements that map as a
header table (and rather naively, at that - linear scan is SO fifties ;) ).

In fact, I argue that the proposed version is actually simpler than the
previous code; it can maybe even be simplified further but not much
further, let's go over it a bit toghether if you want.

struct HttpDigestFieldAttrs is there simply to have a convenient
representation of the header table DigestAttrs. It makes no attempt to be
anything different. Can it be turned into
GenericInitializerFromStringTo<enum foo> ? Yes. Is it worth it for 3 LOC?
not so sure.

DigestFieldslookupTable_t is probably what you are lashing out at, I bet.
Yes, it could be made generic and templatized to represent a
SBuf-to-anything with a static table initializer. I haven't tried that as
that may be PMO and it may preclude turning the index from a map to an
unordered_map with a table-specific hasher (gperf-generated or
hand-written, it's rather trivial to do it in this and in many other cases
we care about).
Doing that with a generic would be harder, require roughly about as much
code, and for a <20-LOC class, would probably be overdesign.

The patch has been build-tested successfully on all platforms we care
about, and run-tested (refactoring went in two phases: add new code,
assert() that it behaves like the old code and run-test, rip old code out).

IMVHO HttpHeaderFieldAttrs is poor legacy, and deserves a thankful and
merciful eviction from our code-base :)


-- 
    Kinkie
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20150726/a2e503cb/attachment.html>