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

[Shively, Gregory]

> Must(false) just throws an exception. Where that exception will be caught
> before main() when thrown from your code, I do not know (perhaps you
> already do). Must(false) is just a trick to check whether all exceptions in your
> code will be caught/handled the way you want. The alternative is to trigger
> real exceptions, but that is often a lot harder.
> 
> In general, we should be using as few try/catch as possible to accomplish the
> desirable outcome. Thus, always adding try/catch "just in case" is the wrong
> approach (I am _not_ saying you are doing that!).
> Until Squid exception handling is improved, testing is often the best way to
> find the right place.
> 
> If you decide that a try/catch is necessary, place it as high as possible and
> please do not forget to print the caught exception. There are examples in the
> code.
> 
> 
> Please note that you do not have to catch exceptions even if they kill Squid.
> The current lack of try/catchers around critical contexts is not your problem.
> Add try/catch only if you think catching close to your new code is necessary
> for your code to work well under more-or-less "normal"
> conditions. I doubt it is currently necessary because you are not using
> exceptions for benign errors.
> 
> 

I think I'll just leave the exception handling for the caller then. There really isn't anything that can be done to resolve the exception in the callee.

> > +    if (n == -1) {
> > +        int xerrno = errno;
> > +        debugs(89, DBG_IMPORTANT, HERE << "Reading from PFCTL failed: "
> << xstrerr(xerrno));
> > +        return false;
> > +    }
> > +
> > +    close(pipefd[0]);
> 
> Now leaking pipefd[0] on errors. Someday, Squid will have a auto-close
> descriptor wrapper...
> 
> 

Thanks - I felt list I was missing something there :-).

> > +        SBuf host;
> > +        int64_t port;
> > +
> > +        if (tk.token(host, bracket) || tk.token(host, colon)) {
> 
> Nice rewrite! You can move the port declaration lower, and you should.
> And no empty lines between the declaration and the single-statement code
> that uses that declaration exclusively:
> 
>     SBuf host;
>     if (...) {
>        int64_t port = 0;
>        if (...) ...
>     }
> 
> 
> > +    while (!tk.atEnd()) {
> > +        if (tk.token(host, bracket) || tk.token(host, colon)) {
> ...
> > +        }
> > +
> > +        tk.skip('\n');
> > +    }
> 
> An infinite loop if the actual input contains, say, a single space and nothing
> else? Or a token without brackets and colons? That is unlikely, of course, but
> perhaps not completely impossible. I am not dictating code, but to skip an
> arbitrary bad line you might need something like this at the end of an
> iteration:
> 
>   // the combination covers all characters so we will make progress
>   (void)tk.skipAll(anythingButLf); // silently skip any garbage
>   (void)tk.skipAll(lf))
>   // either at the beginning of the next line or eof
> 
> 
> BTW, I believe the previous version of your patch had a similar infinite loop
> problem, but now it became obvious, thanks to your nice refactoring.
> 
> 

You are right - there is an infinite loop. I started to play with the tokenizing after I sent the patch and didn't see the infinite loop, but I don't think it is doing what I was attempting in the loop anyway. How about I get rid of the loop all together - I should be only getting one line from pfctl, and if the parsing fails -I should probably just be returning false anyway instead of attempting to find what I expect later in the buffer. I was trying to be safe from the parsing, but I'm starting to think I should just fail out of the parsing if the buffer doesn't have what I expect. 

> Thank you,
> 
> Alex.

A different question - I'm wondering if I should move where this conditional compilation is being placed with regard to the other conditional compilation. I had put it in based on what the last compilation options I had used when attempting to get this working before looking in the code. I didn't know if this would be enabled by configure or some other macro that might be defined on OSX. But it looks like I have it inside the conditional compilation for !USE_NAT_DEVPF. Do you think it should be moved?

Regarding the test_builds.sh - got it to run with the --keep-going and grepped out of the logs - only PASS in the summary section. This was with the current patch. I'll run it again, but at least know that it is working.

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