[squid-dev] [PATCH] Add reply_header_add

[Alex Rousskov]

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.

+1


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)?


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.
>>