[squid-dev] [PATCH] OSX transparent-proxy using pfctl

[Shively, Gregory]

> -----Original Message-----
> From: Alex Rousskov [mailto:rousskov at measurement-factory.com]
> Sent: Thursday, September 29, 2016 4:12 PM
> To: squid-dev at lists.squid-cache.org
> Cc: Shively, Gregory <gregory_shively at fanniemae.com>
> Subject: [EXTERNAL] Re: [squid-dev] [PATCH] OSX transparent-proxy using
> pfctl
> 
> On 09/29/2016 01:12 PM, Shively, Gregory wrote:
> 
> > Sometimes these mailing lists make me think like I'm talking to one
> > person :-).
> 
> Glad we all sound coherent to you :-)!
> 
> 
> > ERROR: files left in build directory after distclean:
> > ./src/cf_gen.dSYM/Contents/Info.plist
> > ./src/cf_gen.dSYM/Contents/Resources/DWARF/cf_gen
> 
> > I can run with the "--keep-going" flag, but that ends with a the
> > equivalent error in btlayer-05-no-deps-esi. Do I run it this way and
> > just grep the logs for FAIL or something?
> 
> Sounds like a good plan to me.
> 
> 

I'll follow that plan then.

> > I wasn't sure if I should handle it or let it flow up, since if it was
> > in an overflow state I would doubt I could handle this packet, but
> > maybe the next connection would be successful.
> 
> I recommend temporary adding an exception with Must(false) inside the
> parsing code and testing what happens. This approach may give you enough
> information to decide whether your code or the caller should be responsible
> for handling parsing exceptions.
> 
> 

If I understand correctly, the Must(false) goes all the way up the stack to main and exits squid. If instead it falls thru and returns false - the connection will fail and the next connection another attempt at parsing. Or is the question that the call to token() or int64() throws an exception and how that is handled, e.g. put a try/catch block around the parsing?

> > +    while ((n = read(pipefd[0], buf, sizeof(buf))) > 0) {
> > +        state.append(buf);
> 
> You need to tell SBuf how much to append(). read(2) does not 0-terminate
> buf!
> 
> 
> > +    if (n == -1) {
> > +        int xerrno = errno;
> 
> Too late: errno may already be different after close().
> 
> 

Thanks.

> > +        // Move to EOL and try again from the next line
> > +        tk.token(host, lf);
> 
> If possible, use one of the skip() methods to avoid the implication that you
> are parsing host here (and to save a couple of CPU cycles).
> 
> Please note that Tokenizer has a method to extract integers. I am not sure it
> will help you here, but it might.
> 

Thanks - I totally missed that method.

> > +                newConn->local = host.c_str();
> > +                newConn->local.port(atol(port.c_str()));
> > +                debugs(89, 5, HERE << "address NAT: " << newConn);
> > +                return true;
> 
> > +                newConn->local = host.c_str();
> > +                newConn->local.port(atol(port.c_str()));
> > +                debugs(89, 5, HERE << "address NAT: " << newConn);
> 
> If you can avoid code duplication, please do so.
> 

Ok.

> > +    CharacterSet bracket("bracket", "[]");
> > +    CharacterSet colon("colon", ":");
> > +    CharacterSet lf("lf", "\n");
> > +    CharacterSet ws("ws", " \t\r\n");
> 
> These should be declared as static to avoid allocating/initializing lots of bytes
> on every access and as constant to help the optimizer (and to avoid future
> bugs). And you can move them inside the loop then -- they are unused
> outside the loop.
> 
> In general, declare variables as late as possible and make them const if
> possible.

Makes sense.

> 
> 
> Thank you,
> 
> Alex.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: squid-3.5.20-osx-devpf-transparent.patch
Type: application/octet-stream
Size: 3327 bytes
Desc: squid-3.5.20-osx-devpf-transparent.patch
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20160929/4636e9c3/attachment.obj>