On 03/14/2016 05:46 PM, Nathan Hoad wrote: > The attached patch implements reply_header_add, for adding HTTP > headers to reply objects as they're sent to the client.
Thank you for this useful addition. Unfortunately, it needs quite a bit of work. * Please _carefully_ review your src/cf.data.pre changes. There appear to be many copy-paste errors there, some of which seriously misleading (e.g., "incoming" vs. "outgoing" and "request" vs. "response"). * 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. * You have not adjusted HTTP response headers produced by tunnelConnected() and ClientHttpRequest::sslBumpStart(). Please either apply the same logic to those headers or explicitly document that successful CONNECT responses are out of this option reach. I also recommend the following adjustments: * The scary "In theory, all of ..." paragraph that made a lot of sense for requests is barely applicable to post-cache responses, where most information is already available. Please consider making that text context-neutral and moving it to an option-independent location in the begging of squid.conf. That would be really useful IMO because we have many options using logformat codes now and that old text is applicable, to various degrees, to all of them. * Adding a sentence or two explicitly stating that this directive does not affect cached responses. Your copied text says that it "has no affect on cache hit detection", and that is also true, if somewhat redundant for responses. * Adding a "See also: request_header_add" statement and the symmetrical statement for request_header_add. > + if (Config.reply_header_add && !Config.reply_header_add->empty()) > + httpHdrAdd(hdr, request, http->al, *Config.reply_header_add); I know you are reusing an existing code template here, but this particular copy starts duplicating a lot of logic. I recommend moving both guards/conditions from the if-statement into httpHdrAdd() itself. If others object to moving both conditions on performance grounds, please move at least the second/empty() condition: That is, ideally: httpHdrAdd(hdr, request, http->al, Config.reply_header_add); Or, if others object: if (Config.reply_header_add) httpHdrAdd(hdr, request, http->al, *Config.reply_header_add); Thank you, Alex. _______________________________________________ squid-dev mailing list squid-dev@lists.squid-cache.org http://lists.squid-cache.org/listinfo/squid-dev