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

[Amos Jeffries]

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.


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

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

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

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


in src/HttpHeader.cc:

* can you move the headerLookupTable to src/http/RegisteredHeaders.cc
and the Http:: namespace as well please ?
 - 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

* 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()

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

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

 - 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

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

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


in http/RegisteredHeaders.h:

* please place the new symbols in the Http:: namespace


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.



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

Amos