[squid-dev] [PATCH] VIA creation code duplication

[Amos Jeffries]

On 11/02/2017 5:04 a.m., Alex Rousskov wrote:
> On 02/09/2017 10:19 AM, Amos Jeffries wrote:
>> On 3/02/2017 4:02 a.m., Eduard Bagdasaryan wrote:
>>> This patch fixes VIA appending code duplication, moving common
>>> code into a separate method.
> 
>> Since Via is a list header we should be able to just append a new Via
>> header to the header list with putStr. No need to use getList, String,
>> delById to inject on the end of existing Via string.
>>
>> Please also take the opportunity to fix a long standing protocol
>> violation bug in Via header produced by Squid:
>>  The version part of the header is only supposed to elide the protocol
>> label if the that label would be "HTTP/" (ie. for messages received via
>> HTTP and HTTPS).
>>
>> eg. for ICY replies:
>>   Via: ICY/1.1 squid
>>
>> eg. for FTP gatewayed replies (or relayed requests):
>>   Via: FTP/2 squid
>>
>>
>> After those calls that require String are gone please assemble the value
>> in an SBuf instead of static char* buffer.
>>
>> Also, for messages where Squid itself generated the message (ie
>> Downloader or ESI) there should be no Via header added at all.
> 
> 
> Also, please fix the 10 oldest open bugs on Squid Bugzilla while you are
> at it.</sarcasm>
> 
> IMHO, you may declare any and all of the changes requested by Amos to be
> outside the scope of the code deduplication patch you have prepared.
> Your patch is valuable even with its current narrow scope. If the patch
> has no problems, it should be acceptable "as is". Amos, if you disagree
> with this assessment, please say so.


This patch is "polishing a turd" as the saying goes and a bit premature.
There are some obvious and easy to fix bugs that should be attended
first, then polishing afterwards.

In these particular pieces of code starting with the de-duplicate
obscures that the code was intended to be doing two quite different
things to begin with.

src/client_side_reply.cc was a wrong implementation of an append to a
list-header, and src/http.cc was an inefficient implementation of a
copy. That results in this patches addVia() method being needlessly
complex and inefficient.


AFAICS there is some duplication, but not the same logic which this
patch is de-duplicating.


If we start with fixing the bugs...

* the code in src/client_side_reply.cc should be just this (no slow
String manipulations):

 /* Append VIA */
 if (Config.onoff.via) {
    LOCAL_ARRAY(char, bbuf, MAX_URL + 32);
    snprintf(bbuf, MAX_URL + 32, "%d.%d %s",
             reply->sline.version.major,
             reply->sline.version.minor,
             ThisCache);
     hdr->putStr(Http::HdrType::VIA, bbuf);
 }


* the code in src/http.cc at first glance looks like its doing the right
thing. It both copies then appends. BUT its operation is also
interdependent with the code inside
copyOneHeaderFromClientsideRequestToUpstreamRequest().

If we make copyOneHeader*() function actually do the copy in all cases:
i.e.
-  case Http::HdrType::VIA:
-       /** \par Via:
-         * If Via is disabled then forward any received header as-is.
-         * Otherwise leave for explicit updated addition later. */
-
-        if (!Config.onoff.via)
-           hdr_out->addEntry(e->clone());
-
-        break;

(incidentally this fixes a bug when Connection:Via is received).


* then src/http.cc chunk labeled "append Via" becomes just a simpler append:

 /* Append VIA */
 if (Config.onoff.via) {
    LOCAL_ARRAY(char, bbuf, MAX_URL + 32);
    snprintf(bbuf, MAX_URL + 32, "%d.%d %s",
             request->http_ver.major,
             request->http_ver.minor,
             ThisCache);
     hdr_out->->putStr(Http::HdrType::VIA, bbuf);
 }

... which is *now* a proper duplicate of the client_side_reply.cc.


* Those two remaining append bits should be what this new addVia()
method does.
 - and addVia() no longer needs the 'from' parameter complexity.


If you still want to do the de-duplicate standalone, fine. I'll do the
bug fixing myself.

Amos