[squid-dev] [PATCH] HttpHeader migration (coverity-fixes branch)

[Kinkie]

On Tue, Aug 4, 2015 at 2:58 PM, Amos Jeffries <squid3 at treenet.co.nz> wrote:

> On 4/08/2015 11:22 p.m., Kinkie wrote:
> > Hi all,
> >   the attached patch is a build- and run-tested merge proposal for next
> > round of the coverity-fixes branch, currently focusing on more effective
> > header name -> id lookups.
> >
> > Here's the status with the current todo checklist:
> >
> >  * DONE 1. shift HDR_BAD_HDR to end of enum
> >  * DONE 2. shift headers data array to http/RegistredHeaders.cc
> >  * DONE 3. creatign LookupTable object from teh enum and array
> >  * (with HDR_BAD_HDR as invalid value)
> >  * DONE 4. replacing httpHeaderIdByName() uses with the lookup table
> >  * NOT POSSIBLE 5. merge HDR_BAD_HDR and HDR_ENUM_END into one thing -
> > HDR_ENUM_END is overloaded meaning "All" headers in Manglers.
> >  * DONE 6. replacing httpHeaderNameById with direct array lookups
> >  * DONE 7. being looking at the other arrays removal
> >
> > In working on this I found out several instances of enum abuses -
> tracking
> > those down has been the hardest part of the effort.
> > HttpHeader::parse is being used to parse error page templates - thus the
> > relaxed any_registered_header() checks in some methods, e.g.
> > HttpHdeader::addEntry().
> >
> > Next steps, if there is consensus:
> > - moving LookupTable away from std::map to std::hash with custom
> > gperf-derived Hashers for extra boost
> > - investigating whether strongly-typed enums can be used instead of
> C-style
> > enums in more places.
> > - moving away from homegrown bitfields (CBIT_TEST etc.) towards
> > std::vector<bool> or std::vector<unsigned char>, possibly via a class
> > bitfield or somesuch.
> >
>
> audit results:
>
> * httpHdrCcCleanModule() and httpHdrScCleanModule() can both be fully
> deleted now.
>

Yes. I chose to let them in: they are NOPs, not in the critical path, and
may be useful in the future. Let me know if you still want them removed.

in src/enums.h:
>
> * please move the altered enums out to the HttpHdr{C,S}c.h files
>  - or worst-case to http/forward.h
>

Done.


> in src/base/LookupTable.h:
> * missing empty line separator between SBufCaseInsensitiveLess and the
> LookupTable template
>

Done

>
> * does SBufCaseInsensitiveLess have to be a struct?
>  - inheritence and operators is a bit odd with struct
>

Turned to class.


> * LookupTableRecord should be a class.
>  - custom storage types can then inherit from it, with it as the first
> parent
>

Well, templatization can prevent that need, but sure. Should I implement
that?
Isn't nowadays a struct == all-public class though? It looks odd, but
functionally is the same


> in src/HttpHeader.cc:
>
> * can you move the headerLookupTable to src/http/RegisteredHeaders.cc
> and the Http:: namespace as well please ?
>

Moved to RegisteredHeaders and renamed HDR_FOO to Http::HdrType::FOO .
Moving to a strongly-typed enum is unfortunately not feasible as eCAP
requires integer-enum equivalence; it may be that the whole change has to
be reverted.


>
>  - that way we keep the polished new code in http/libhttp and the old C
> code in src/*.cc files.
>



>
> * headerLookupTable is a global / static. ie. HeaderLookupTable.
>  - or when in the namespace ... Http::HeaderLookupTable
>

Done.


> * any_registered_header() is wrong.
>  - it matches for HDR_OTHER which by definition is a non-registered header
>  - assert_eid() is equivalent to any_valid_header()
>

Then we need to change name. We need two semantics:
1) any header which is valid and defined (including OTHER)
2) any header ID which will not go out-of-bounds (same as (1) + HDR_BAD_HDR)

any suggestion?


>
> * "// is this really needed?"
>  - either make that a TODO or remove. as a comment it is useless and
> will only get lost.
>

Comment removed.


> * the chunk @1528
>  - s/ "got hdr id hdr: " / "got hdr-id=" /
>

done


>
>  - please consider removing the assert(assert(any_valid_header(id)))
>    it could probably be replaced by:
>       if (!assert(any_valid_header(id)))
>           id = HDR_OTHER
>

Huh? I can't find any nested assert


> * I think the wrong assert() was removed from the
> HttpHeaderEntry::get*() methods.
>  - since the entry exists there should be no use to asserting its id is
> valid.
>  - please consider replacing the two asserts with an if() actually
> handling of the case where id is BAD_HDR_NUM or not the right header
> value-type. By returning some invalid value-type representation instead
> of asserting or trying to parse.
>

Removed the asserts; I am convinced they are just wasted cycles right now.


>
> * can valid_id in httpHeaderFieldStatDumper() be made a const bool ?
>

Yes, done.


> in http/RegisteredHeaders.h:
>
> * please place the new symbols in the Http:: namespace
>

done, except for operator<<(Http::HdrType)


> in src/HttpHeaderTools.cc:
>
> * HeaderManglers::find contains "e.id != HDR_OTHER && e.id < HDR_ENUM_END"
>  - that should now be just: e.id != HDR_OTHER && any_registered_header(
> e.id)
>  - once you fix any_registered_header definition the !=OTHER check may
> not be necessary.
>

moved to Http::any_registered_header


> Okay. I think thats all. But I may have missed stuff. SO at least
> another round ahead :-(
>

Here it is, to lighten up your morning :)

Thanks!

-- 
    Francesco
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20150804/ace96b56/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: coverity-fixes-3-v2.patch
Type: text/x-patch
Size: 270962 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20150804/ace96b56/attachment-0001.bin>