[squid-dev] [PATCH] Coverity(-inspired) fixes part four, HttpHeader refactor

[Kinkie]

On Thu, Aug 20, 2015 at 8:46 AM, Amos Jeffries <squid3 at treenet.co.nz> wrote:

> On 16/08/2015 7:09 p.m., Kinkie wrote:
> > Hi all,
> >   the attached patch does:
> > - EnumIterator, which now uses autoconf to detect std::underlying_type
> > - Removes header masks in HttpHeader.cc and HttpReply in favor of..
> > - gperf-generated header table with flags describing headers all
> collected
> > in one place
> > - replaces HttpHeader::getEntry with explicit vector iteration
> > - Implements and uses Http::HeaderTable.lookup(string-or-Http::HdrType)
> to
> > look up headers in the gperf-generated hash
> > - small optimizationi in HttpHeader::getLastEntry thanks to reverse
> > iterators
> > - use erase-remove pattern in HttpHeader::compact in place of resize()
> > - use std:: algorithms instead of explicit loop in HttpHeader::delById
> > - gets rid of httpHeaderCalcMask - masks are still used elsewhere but
> > generating them is now easier thanks to WholeEnum<type>
> > - some readability improvements
> > - refactor HeaderManglers::track to avoid overloading of meaning the
> > enumEnd_ enumerator - please help me checking carefully I haven't altered
> > functionality here
> > - implements a case-insensitve hash function for SBuf
> > - all known headers are now unconditionally parsed. May not be acted upon
> > if some functions are not compiled in
> > - LookupTable now uses an unordered_map instead of map for backing
> storage.
> > No user-visible change for SBuf and std::string users unless they wish to
> > have a custom hasher
> >
> > Most code has been run-tested (most notable exception: HeaderManglers),
> > farm-build-, coadvisor- and polygraph- tested. The main focus of this
> > refactoring is correctness and readability, c++11-ification and
> recovering
> > from the performance regression that was introduced ~10 days ago.
> >
>
>
> Here goes. Audit:
>
>
> * IMO the HeaderManglers really do need at least basic testing with
> these changes. A regression there affects widely used features.
>
> basic tests are easy enough. A simple proxy altering the Server header
> to a fixed value, adding some custom new header, and stripping something
> common like Expires should be sufficient to trigger any code problems
> and eyeball any major issues from the squidclient output to verify working.
>
>
After some careful checking:
*_header_access and *_header_add work fine.
*_header_replace is broken, but it's been for a long time (I run-tested
with code from february).


> in acinclude/* and configure.ac:
>
> * tests and macro defines for C++11/0x back-compat are in
> acinclude/ax_cxx_0x_types.m4
>

The file name seems inappropriate, but sure.
Done.


> in src/HttpHeader.cc:
>
> * please use {} on the for in HttpHeader::append().
>  - aids readability of multiple nesting layers
>  - same occurs elsewhere, I cant see the method name.
>

findEntry, findLastEntry, refreshMask, getList. Done.


> * "assert(! Http::HeaderLookupTable.lookup" looks - odd.
>  - the extra whitespace does not really add anything and detracts from
> searching for "!Http::HeaderLookupTable.lookup"
>

Changed.


> * inconsistent changes ofr NULL->nullptr
>  - eg in HttpHeader::findLastEntry() NULL is not changed, but elsewhere
> it usually is.
>

That's actually intentional: I changed that only when something in the
vicinity changed so not to make the patch any more enormous. I can blanket
change if that's preferred.


> in src/HttpHeaderTools.cc:
>
> * HeaderManglers::dumpReplacement() has some odd line wrapping in the
> parameter lists of header_mangler_dump_replacement()
>

Removed all line wrapping.


> in src/Makefile.am:
>
> * for testEnumIterator please add $(SQUID_CPPUNIT_LA) to the *_LDADD
> list instead of a *_DEPENDENCIES entry
>

hm.. it's applied inconsistently. Documentation (at line 957) says to have
it in both places.
testUfs, testRock : do not have it as a dependency
testHttpReply, testBoilerplate, testCacheManager, testDiskIO, testEvent,
testEventLoop, test_http_range, testHttp1Parser, testHttpRequest,
testStore, testString, testUrl, testSBuf, testSBufList, testConfigParser,
testStatHist (and so on..) have it as a dependency
testACLMaxUserIP : used to have it as a dependency (but now it's commented
out)

For now I'm following documentation and leaving it in both places; if you
have an opinion about how it should be I'll gladly carpet-apply that to the
whole file.


> * also please place tests/stub_* and base/EnumIterator.h in the nodist_*
> list from now on.
>

Ok. Changed documentation at about line 951 to reflect that.


>  - the test _SOURCE list should now only contain the tests/test*.{h,cc}
> actually containing the unit-test code.
>
Does this also apply to the files actually being tested? (X.h, X.cc)
I am fine either way, I would just like it to be reflected in the
documentation. Not changed there for now, just in the testEnumIterator part.


>  - this helps prevent unit tests being the only reason files are dist
> (found a few like that already), so we can ensure they are linked to
> Squid main binary or libraries properly for automake dependency detection.
>
>
> in src/SBufAlgos.cc:
>
> * whats cctype needed for?
>

tolower(3).


>
> in src/SBufAlgos.h:
>
>  * please doxygen comment new class CaseInsensitiveSBufHash
>

Done.


> in src/adaptation/icap/ModXact.cc:
>
> * why is #if USE_ADAPTATION wrappers being added?
>  - this entire file and even libicap.la should never be linked if
> adaptation is disabled.
>

By mistake. Thanks for catching this.


> in src/base/EnumIterator.h:
>
> * please place the #if wrappers underneath the copyright blurb.
>  - ensures the squid copyright is part of any code using that squid
> header, and
>  - makes the maintenance tools detect it as ours not a third-party
> copyright.
>  - same for tests/testEnumIterator.h
>

Ok.


>
> * please doxygen comment new class EnumIteratorBase, ReverseEnumIterator,
>  - eg, do these cover all values?
>    where does it stop iterating?
>    what value is produced for out-of-bounds ?
>    uni or bi-directional ?
>

Done.

>
> * for EnumRangeT and WholeEnum
>  - utilize 'empty' lines between paragraphs
>  - \note for highlighting additional notes
>  - highlight example code with \code ... \endcode).
>

Done

>
> * s/Enum/EnumType/
>  - to avoid any confusion with keyword 'enum'
>

Done.


> in src/base/Makefile.am:
>
> * alphabetical order.
>

Done.

>
>
> in src/http.cc:
>
> * please mark use of name.c_str() with "XXX: performance regression,
> c_str() reallocates"
>

Actually, no in this case it doesn't unless ilen is exactly equal to one of
the MemBlob MemPool sizes (possible but unlikely, I estimate in the 0.1%
range).
There are some performance deltas but they are IMO quite marginal:
slowdowns: SBuf housekeeping, temp creation (of ilen+1 bytes) done in two
steps instead of one
speedups: SBuf uses mempools, "*" case is checked before lowering case


>  - the xmalloc removal is countered by the data-copy into "SBuf name".
> Making this a new second data-copy.
>
>
> in src/http/RegisteredHeaders.h:
>
> * please lock ACCEPT in as first entry;
> "
>   enumBegin_ = 0,
>   ACCEPT = enumBegin_,
>   ...
> "
>

It already was, by assigning both enumBegin and ACCEPT value 0. Changed for
clarity's sake.


> * please delimit the methods and members of HeaderTableRecord with an
> empty line (and maybe another "public:")
>

Done.


> * a formatter run is going to be needed. Things like enum HdrKind
> contain tab-indented lines and double-empty lines.
>

Yes.


>
> * HeaderLookupTable_t
>  - need empty lines between paragraphs in doxygen comments again.
>  - ie before the paragraph starting "Actual records are ..."
>
> * "///look record ..." --> "/// look ..."
>
> Ok


>
> in src/http/RegisteredHeadersHash.cci:
>
> * is there any way to add a comment blurb "/* AUTO-GENERATED DO NOT EDIT
> */" to the top of this file ?
>

It can and I did, but it's not possible at the very top unfortunately.


>
> * also need to add the copyright boilerplate
>

Same thing.


>
>
> in  src/http/RegisteredHeadersHash.gperf:
>
> * need a way to add the copyright boilerplate as a gperf-ignored comment
>

Done that. There is a preamble section into which I copied the C++ comments
above.


>
>
> in  src/tests/testEnumIterator.cc:
>
> * include path "testEnumIterator.h" -> tests/testEnumIterator.h
>

Done.


> * test enums should also use the " first = enumBegin_, " trick
>

Done.


> * testBidirectionalIter() also needs to test for reaching out-of-bounds
> conditions in either direction.
>  - can it go fully across begin() -> end() -> begin() successfully in
> only the expected count of iterations? (no more, no less)
>

Ok, added and tested.

Updated patch attached.


-- 
    Francesco
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20150821/f9d285fb/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: coverity-fixes-4-v2.patch
Type: application/octet-stream
Size: 175001 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20150821/f9d285fb/attachment-0001.obj>