[squid-dev] [PATCH] Add reply_header_add

[Nathan Hoad]

Hello,

Attached is an updated patch with Alex's suggested changes, i.e.
moving the use of httpHdrAdd into httpHdrMangleList, and moving some
code out of httpHdrMangle into httpHdrMangleList.

I've opted to remove Config2.onoff.mangle_request_headers completely.
I found keeping it around and making use of it in httpHdrMangleList
made the flow of execution confusing, with regards to ensuring
httpHdrAdd was still called.

Thank you for taking the time to look at this.

Nathan.

On 16 March 2016 at 14:15, Alex Rousskov
<rousskov at measurement-factory.com> wrote:
> On 03/15/2016 07:29 PM, Nathan Hoad wrote:
>> On 15 March 2016 at 12:11, Alex Rousskov wrote:
>>> On 03/14/2016 05:46 PM, Nathan Hoad wrote:
>
>>> * You have not adjusted HTTP response headers produced by
>>> Http::One::Server::writeControlMsgAndCall(). Please either apply the
>>> same logic to those headers or explicitly document that control message
>>> headers are out of this option reach.
>
>> Done.
>
>
> Thank you. After those improvements, you can probably see a new pattern
> emerging:
>
>> if (Config2.onoff.mangle_request_headers)
>>     httpHdrMangleList(hdr_out, request, ROR_REQUEST);
>>
>> httpHdrAdd(hdr_out, request, al, Config.request_header_add);
>
> and
>
>
>> httpHdrMangleList(&rep->header, http->request, ROR_REPLY);
>> httpHdrAdd(&rep->header, http->request, http->al, Config.reply_header_add);
>
> and
>
>> httpHdrMangleList(hdr, request, ROR_REPLY);
>>
>> httpHdrAdd(hdr, request, http->al, Config.reply_header_add);
>
> and
>
>> $ bzr grep '    httpHdrMangleList' | wc -l
>> 3
>
> and, I would imagine,
>
>> bzr grep '    httpHdrAdd' | wc -l
>> 3
>
>
> Yes, there are only three cases, and in all of them, we apply the same
> set of two operations. Thus, you can now move the httpHdrAdd() call into
> httpHdrMangleList()!
>
> If you do that, move the code that gets the right Config.*_header_access
> mangler from the beginning of httpHdrMangle() to httpHdrMangleList()
> itself and adjust it to also check Config2.onoff.mangle_request_headers
> during ROR_REQUEST.
>
> Adjust as needed: httpHdrMangle() is a private/static function and you
> will be working with all three httpHdrMangleList() calls in existence
> (AFAICT).
>
>
> I cannot require these additional changes from you, but if you need an
> extra incentive/motivation, then just look at the loop in
> httpHdrMangleList() and what happens at the start of httpHdrMangle()
> that the loop calls on every iteration. Consider the common case of no
> Config.*_header_access manglers configured. See how httpHdrMangleList()
> always iterates through all header fields while repeating the same
> checks and doing nothing useful at all?
>
> With the above changes (that move those checks to be done once, in front
> of the loop, and that will entirely eliminate looping in the common
> case), you would be making Squid a tiny bit faster despite adding a new
> feature!
>
>
> Thank you,
>
> Alex.
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: reply-header-add-v3.patch
Type: text/x-patch
Size: 10521 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20160317/0e544e27/attachment.bin>