[prev in list] [next in list] [prev in thread] [next in thread] 

List:       squid-dev
Subject:    Re: [PATCH] Proper assignment and copying of HttpHeader
From:       Amos Jeffries <squid3 () treenet ! co ! nz>
Date:       2011-02-18 6:30:39
Message-ID: 4D5E120F.90104 () treenet ! co ! nz
[Download RAW message or body]

On 18/02/11 18:08, Alex Rousskov wrote:
> Code cleanup: Proper assignment and copying of HttpHeader.
>
> Besides being the Right Thing, having correct assignment operator and
> copy constructor helps classes that have HttpHeader data members to
> avoid defining explicit assignment operators and copy constructors.
>
> Also adds forgotten reset of "len" in the clean() method. Perhaps that
> data member is not needed at all?
>
> Other polishing touches. HttpHeader::reset() is now a tiny bit faster.
>
> No runtime changes expected.
>
>
> Thank you,
>
> Alex.
> P.S. I stumbled upon this ancient mess while enhancing adaptation
> History classes. This patch will remove a few dozen lines from those
> classes, but those changes will be submitted separately.

+1 on this patch going in.


Regarding "len"...

Doxygen shows references by
   HttpHeader::addEntry() - updated, not used.
   HttpHeader::delAt() - updated, not used.
   HttpHeader::insertEntry() - updated, not used.
   peerDigestRequest() - just an assert. which is abusing it anyway to 
check that zero headers are stored.

   HttpRequest::prefixLen() - the only place that actually uses it.

There are only two places that use prefixLen() in turn

  peerDigestFetchSetStats
   - has notes about it being the wrong metric to use.

  clientReplyContext::traceReply
   - uses it to set the BODY content length of the trace reply. This 
appears to be another bug since the request-line and any entity body 
data are not accounted for.
   The raw input block from the ConnStateData buffer is what should be 
sent back right? in which case the size of that can be used without 
re-packing anything.


So overall yes I believe it should go. But trace and digest needs fixing 
before that can happen.

Amos
-- 
Please be using
   Current Stable Squid 2.7.STABLE9 or 3.1.11
   Beta testers wanted for 3.2.0.5
[prev in list] [next in list] [prev in thread] [next in thread]