<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Aug 4, 2015 at 2:58 PM, Amos Jeffries <span dir="ltr"><<a href="mailto:squid3@treenet.co.nz" target="_blank">squid3@treenet.co.nz</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div>On 4/08/2015 11:22 p.m., Kinkie wrote:<br>
> Hi all,<br>
>   the attached patch is a build- and run-tested merge proposal for next<br>
> round of the coverity-fixes branch, currently focusing on more effective<br>
> header name -> id lookups.<br>
><br>
> Here's the status with the current todo checklist:<br>
><br>
>  * DONE 1. shift HDR_BAD_HDR to end of enum<br>
>  * DONE 2. shift headers data array to http/RegistredHeaders.cc<br>
>  * DONE 3. creatign LookupTable object from teh enum and array<br>
>  * (with HDR_BAD_HDR as invalid value)<br>
>  * DONE 4. replacing httpHeaderIdByName() uses with the lookup table<br>
>  * NOT POSSIBLE 5. merge HDR_BAD_HDR and HDR_ENUM_END into one thing -<br>
> HDR_ENUM_END is overloaded meaning "All" headers in Manglers.<br>
>  * DONE 6. replacing httpHeaderNameById with direct array lookups<br>
>  * DONE 7. being looking at the other arrays removal<br>
><br>
> In working on this I found out several instances of enum abuses - tracking<br>
> those down has been the hardest part of the effort.<br>
> HttpHeader::parse is being used to parse error page templates - thus the<br>
> relaxed any_registered_header() checks in some methods, e.g.<br>
> HttpHdeader::addEntry().<br>
><br>
> Next steps, if there is consensus:<br>
> - moving LookupTable away from std::map to std::hash with custom<br>
> gperf-derived Hashers for extra boost<br>
> - investigating whether strongly-typed enums can be used instead of C-style<br>
> enums in more places.<br>
> - moving away from homegrown bitfields (CBIT_TEST etc.) towards<br>
> std::vector<bool> or std::vector<unsigned char>, possibly via a class<br>
> bitfield or somesuch.<br>
><br>
<br>
</div></div>audit results:<br>
<br>
* httpHdrCcCleanModule() and httpHdrScCleanModule() can both be fully<br>
deleted now.<br></blockquote><div><br></div><div>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.<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
in src/enums.h:<br>
<br>
* please move the altered enums out to the HttpHdr{C,S}c.h files<br>
 - or worst-case to http/forward.h<br></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
in src/base/LookupTable.h:<br>
* missing empty line separator between SBufCaseInsensitiveLess and the<br>
LookupTable template<br></blockquote><div><br></div><div>Done </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
* does SBufCaseInsensitiveLess have to be a struct?<br>
 - inheritence and operators is a bit odd with struct<br></blockquote><div><br></div><div>Turned to class.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
* LookupTableRecord should be a class.<br>
 - custom storage types can then inherit from it, with it as the first<br>
parent<br></blockquote><div><br></div><div>Well, templatization can prevent that need, but sure. Should I implement that?</div><div>Isn't nowadays a struct == all-public class though? It looks odd, but functionally is the same</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
in src/HttpHeader.cc:<br>
<br>
* can you move the headerLookupTable to src/http/RegisteredHeaders.cc<br>
and the Http:: namespace as well please ?<br></blockquote><div><br></div><div>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.<br><br></div> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
 - that way we keep the polished new code in http/libhttp and the old C<br>
code in src/*.cc files.<br></blockquote><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
* headerLookupTable is a global / static. ie. HeaderLookupTable.<br>
 - or when in the namespace ... Http::HeaderLookupTable<br></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
* any_registered_header() is wrong.<br>
 - it matches for HDR_OTHER which by definition is a non-registered header<br>
 - assert_eid() is equivalent to any_valid_header()<br></blockquote><div><br></div><div>Then we need to change name. We need two semantics:</div><div>1) any header which is valid and defined (including OTHER)</div><div>2) any header ID which will not go out-of-bounds (same as (1) + HDR_BAD_HDR)<br><br></div><div>any suggestion?<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
* "// is this really needed?"<br>
 - either make that a TODO or remove. as a comment it is useless and<br>
will only get lost.<br></blockquote><div><br></div><div>Comment removed. <br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
* the chunk @1528<br>
 - s/ "got hdr id hdr: " / "got hdr-id=" /<br></blockquote><div><br></div><div>done<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
 - please consider removing the assert(assert(any_valid_header(id)))<br>
   it could probably be replaced by:<br>
      if (!assert(any_valid_header(id)))<br>
          id = HDR_OTHER<br></blockquote><div><br></div><div>Huh? I can't find any nested assert<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
* I think the wrong assert() was removed from the<br>
HttpHeaderEntry::get*() methods.<br>
 - since the entry exists there should be no use to asserting its id is<br>
valid.<br>
 - please consider replacing the two asserts with an if() actually<br>
handling of the case where id is BAD_HDR_NUM or not the right header<br>
value-type. By returning some invalid value-type representation instead<br>
of asserting or trying to parse.<br></blockquote><div><br></div><div>Removed the asserts; I am convinced they are just wasted cycles right now.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
* can valid_id in httpHeaderFieldStatDumper() be made a const bool ?<br></blockquote><div><br></div><div>Yes, done.<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
in http/RegisteredHeaders.h:<br>
<br>
* please place the new symbols in the Http:: namespace<br></blockquote><div><br></div><div>done, except for operator<<(Http::HdrType)<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
in src/HttpHeaderTools.cc:<br>
<br>
* HeaderManglers::find contains "<a href="http://e.id" rel="noreferrer" target="_blank">e.id</a> != HDR_OTHER && <a href="http://e.id" rel="noreferrer" target="_blank">e.id</a> < HDR_ENUM_END"<br>
 - that should now be just: <a href="http://e.id" rel="noreferrer" target="_blank">e.id</a> != HDR_OTHER && any_registered_header(<a href="http://e.id" rel="noreferrer" target="_blank">e.id</a>)<br>
 - once you fix any_registered_header definition the !=OTHER check may<br>
not be necessary.<br></blockquote><div><br></div><div>moved to Http::any_registered_header<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Okay. I think thats all. But I may have missed stuff. SO at least<br>
another round ahead :-(<br>
</blockquote><div><br clear="all"></div></div>Here it is, to lighten up your morning :)<br><br></div><div class="gmail_extra">Thanks!<br></div><div class="gmail_extra"><div><br></div>-- <br><div>    Francesco</div>
</div></div>