[squid-dev] [PATCH] Add reply_header_add

[Nathan Hoad]

On 17 March 2016 at 13:33, Alex Rousskov
<rousskov at measurement-factory.com> wrote:
> On 03/16/2016 05:40 PM, Nathan Hoad wrote:
>
>> I've opted to remove Config2.onoff.mangle_request_headers completely.
>
> Even better! I did not realize it is not a "real" configuration option
> but a silly(?) cache for "Config.request_header_access != NULL".
>
>
>> -httpHdrMangle(HttpHeaderEntry * e, HttpRequest * request, int req_or_rep)
>> +httpHdrMangle(HttpHeaderEntry * e, HttpRequest * request, HeaderManglers *hms, int req_or_rep)
>
> I do not think you need/use the last parameter, but its removal can be
> done when committing your patch.

Good catch! To lessen work for the committer, I've attached a version
of the patch with that change.

>
> +1

Thank you!

>
>
> BTW, do you know whether ACLs are required for HeaderWithAclList (i.e.,
> our "ACLs may be specified" wording is misleading) or optional (i.e.,
> our "Usage: ... field-value acl1 [acl2]" is wrong)?

The usage statement was wrong, as it turns out. I tested the following
configuration:

request_header_add Test-Header "true"
reply_header_add Test-Header "true"

And the headers are attached to the HTTP messages, so ACLs are indeed
optional. In the attached patch I've also updated the Usage statement
for both request_header_add and reply_header_add to reflect reality.

Thank you,

Nathan.


>
>
> Thank you,
>
> Alex.
>
>
>> 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-v4.patch
Type: text/x-patch
Size: 10920 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20160317/58705e9e/attachment.bin>