Hi Willy,

If you make some changes to what you think/know is better and break the change into two parts is fine for me.

About calling setsockopt multiple times, i think the "ret |= " would not evaluate the call behind it if "ret" already is 1, not absolutely sure about that.. I didn't think of starting a if statement with "0 ||" which might speed it up a clock tick or two so would be better anyway instead of having a variable assignment in between.

Thanks, could you let me know when its ready then ill give it another compile&check on FreeBSD. And provide a little 'documentation' on how i configured the 'ipfw' firewall/nat to make it work.

p.s.
Ive spotted a issue in my patch with the IPv6 part where i forgot about the OpenBSD part (SOL_SOCKET & SO_BINDANY) should probably be added there also.

PiBa-NL

Op 8-5-2013 20:18, Willy Tarreau schreef:
Hi,

On Wed, May 08, 2013 at 07:34:19PM +0200, PiBa-NL wrote:
Hi Willy,

Could you please let me know what your findings are about the proposed
patch?
I was on it this afternoon (didn't have time earlier) :-)

I haven't finished reviewing it yet, because I was trying to figure if
there would be an easy way to merge the CTTPROXY mode into the other
transparent proxy options, but I'm not sure that's really useful.

Also I found one issue here :

+                       int ret = 0;
+                       #if defined(SOL_IP)       && defined(IP_TRANSPARENT)
+                       ret |= setsockopt(fd, SOL_IP, IP_TRANSPARENT, &one, 
sizeof(one)) == 0;
+                       #endif
+                       #if defined(SOL_IP)       && defined(IP_FREEBIND)
+                       ret |= setsockopt(fd, SOL_IP, IP_FREEBIND, &one, 
sizeof(one)) == 0;
+                       #endif
+                       #if defined(IPPROTO_IP)   && defined(IP_BINDANY)
+                       ret |= setsockopt(fd, IPPROTO_IP, IP_BINDANY, &one, 
sizeof(one)) == 0;
+                       #endif
+                       #if defined(SOL_SOCKET)   && defined(SO_BINDANY)
+                       ret |= setsockopt(fd, SOL_SOCKET, SO_BINDANY, &one, 
sizeof(one)) == 0;
+                       #endif
+                       if (ret)

As you can see, if we have multiple defines, we'll call setsockopt multiple
times, which we don't want. I was thinking about something like this instead :

        if (0
#if cond1
             || setsockopt(fd, SOL_IP, IP_TRANSPARENT, &one, sizeof(one)) == 0
#endif
#if cond2
             || setsockopt(fd, SOL_IP, IP_TRANSPARENT, &one, sizeof(one)) == 0
#endif
            )
        ...

I'm still on it right now, to ensure we don't break anything.

Does it need some more work, is it implemented wrongly, or would it help
if i send my current haproxy.cfg file?

If i need to change something please let me know, thanks.
I do not think so, I can easily perform the changes above myself, I won't
harrass you with another iteration. Overall it's good but since we're
changing many things at once, I'm cautious. I'd prefer to break it in
two BTW :
   1) change existing code to support CONFIG_HAP_TRANSPARENT everywhere
   2) add FreeBSD support

But if that's OK for you, I'll simply perform the small adjustments
before merging it.

Cheers,
Willy



Reply via email to