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

[Alex Rousskov]

On 09/30/2016 09:04 AM, Shively, Gregory wrote:

> How about I get rid of the loop all together

All other factors being equal, a single statement is better than a loop
with a similar statement inside.

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

I do not know enough about pfctl and the corresponding environment to
answer your question with certainty but it looks like your awk command
should should protect Squid from any irrelevant/garbage lines. If nobody
else speaks up, this is your decision to make.


> +    cmd.append("\" && $8 == \"");
> +    cmd.append("ESTABLISHED:ESTABLISHED\" {print $5}'");

These should probably be merged into one. If no other changes are
needed, this merge can be done during commit.

> +        int xerrno = errno;

> +    pid_t pid = fork();

> +        int xerrno = errno;

> +        int xerrno = errno;

All should be "const". If no other changes are needed, these "const"s
can be added during commit.


> +            debugs(89, 5, HERE << "address NAT: " << newConn);

No HERE in trunk, but it can be removed during commit.

I also recommend showing "state" contents in case you are returning
false because the PFCTL output did not match your expectations.
Something like:

    debugs(89, 3, "no address in" <<
           Raw("PFCTL output", state.rawContent(), state.size());
    return false;

That will cover the important case of no/empty output as well.

> +        enter_suid();
> +        execl("/bin/sh", "/bin/sh", "-c", cmd.rawContent(), NULL);
> +        leave_suid();
> +        exit(EXIT_FAILURE);

I wonder whether we should [save and] return the errno here instead of
generic EXIT_FAILURE?


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

Sorry, I do not know. Amos may know the answer to this question.


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

Excellent.

I have no further comments about the code. Thanks a lot for polishing it!

Alex.