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

[Shively, Gregory]

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

Sounds good. I've gone ahead and made the changes and including.
 
> > +        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.
> 
> 

Will do.

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

Removed the HERE out of all the additional debugs() and included the log entry for not matching an address.

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

Sure - also added a waitpid() to get the status and logged the errno, if the address wasn't found.

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

Thanks. Will see if Amos has some direction.

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

Thanks for helping me with the polish.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: squid-3.5.20-osx-devpf-transparent.patch
Type: application/octet-stream
Size: 3369 bytes
Desc: squid-3.5.20-osx-devpf-transparent.patch
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20161003/014b609f/attachment.obj>