Re: Patch Submission - Header Manipulation

On Fri, 2004-08-27 at 15:57 -0400, Matt Benjamin wrote:
> Dear Squid Folk,
>
> I've implemented a mechamism to arbitrarily add/change/delete headers,
> using an extension to the existing redirect_program helper application.
>
> Attach please find a unified diff made against the 20040805 Squid-3.0
> snapshot--it should patch more recent ones, of course. Also, please
> find a simple example of a redirect program which uses the mechanism to
> add a custom header, which shows the basic idea.

this is quite interesting. There are a few (minor) things that need to
be done to get it into squid 3.0...

You need to add your name to the AUTHORS file, as your contribution is
long enough to be copyrighted on its own.

Can you do the diff with (IIRC) -ko which will remove the $Id$ changes,
which will cause conflicts. Alternatively, I seem to recall Henrik
having a script to do that on devel.squid-cache.org.

in HttpHeader.cc, when you make the function non-static, move the
declaration into HttpHeader.h. Then in redirect.cc you don't have to
duplicate the prototype (which can lead to bugs later if the primary
prototype changes. (C linkage doesn't change for different parameter
lists..) - prototypes should only be placed once, anywhere in squid).

We currently don't use the STL, as it was felt to be a portability
problem. We have a dlink_list abstraction, and a String class. I don't
think we have a pair at the moment, you may need to whip one up for
this. the API for String should be pretty close to std::string, have a
look in SquidString.h.

parse_header_line doesn't seem to handle headers that include : in their
values... maybe I'm missing something ?

Following are some style comments, to help keep the squid code lean 'n'
mean :}.

in your hunk beginning at line 69 of redirect.cc, please put TODO in
caps, thats so that vim etc highlight it.

the block you've added
if (Config.redirectHeaders) {
...
}
should be in its own function. something like:
if (Config.redirectHeaders)
  redirectHeaders(r, reply);

where you clear all the headers, I think you want to just call
httpHeaderClean(&r->client_request->header);

Are the headers you decode from the base64 block a standard set of http
headers? If so, you should just use the built in squid parsing logic:
httpHeaderParse(&r->client_request->header, hdr_temp, hdr_temp +
hdr_temp.size());

likewise, where you render the headers, you seem to be doing so in
standard style, so:
MemBuf mb;
Packer p;
packerToMemInit(&p, mb);
httpHeaderPackInto(&r->client_request->header, &p);
packerClean(&p);
/* append header terminator */
memBufAppend(mb, crlf, 2);
...followed by your base64 encoding :]

nearly done!

this hunk:
+ if(Config.redirectHeaders) {
+ snprintf(buf, buf_sz, "%s %s/%s %s %s %s\n",
+ r->orig_url,
+ inet_ntoa(r->client_addr),
+ fqdn,
+ r->client_ident[0] ? rfc1738_escape(r->client_ident) :
dash_str,
+ r->method_s,
+ hdr_block.buf());
+
+ } else {
+ snprintf(buf, buf_sz, "%s %s/%s %s %s\n",
+ r->orig_url,
+ inet_ntoa(r->client_addr),
+ fqdn,
+ r->client_ident[0] ? rfc1738_escape(r->client_ident) :
dash_str,
+ r->method_s);
+ }
smells wrong to me - too much duplication. I suggest something like:
String headers;
if (Config.redirectHeaders) {
    headers=" ";
    headers.append(hdr_block.buf());
    }
snprintf(buf, buf_sz,"%s %s/%s %s %s %s\n",
         r->orig_url,
         inet_ntoa(r->client_addr),
         fqdn,
         r->client_ident[0] ? rfc1738_escape(r->client_ident) : \
dash_str,
         r->method_s,
         headers.buf());
Equally, you could just use hdr_block for that.

If you can update the patch to address what I've raised above, I'll be
happy to include it in squid-3.0!

Cheers,
Rob