-
Notifications
You must be signed in to change notification settings - Fork 228
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support transparent proxy related unix socket options as possible #510
base: master
Are you sure you want to change the base?
Conversation
285d1fb
to
f9ae19c
Compare
Will this request cause any problems? If so, please let me know. Otherwise, should I close this PR? I don't want to keep it open for a long time. |
Sorry I missed this pr, will try to review this later this week. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you split this up into multiple commits?
- the move to
src/sys/unix.rs
- adding target_os = "fuchsia"
- adding
(set_)ip_transparent_v6
- adding
(set_)ip_bindany_v4
(instead ofip_bindany
) - adding
(set_)ip_bindany_v6
- deprecate
ip_transparent
in favour ofip_transparent_v4
, we're slowly moving to_v4
and_v6
versions
OK, do this work later. |
@Thomasdezeeuw Quick question, |
bd2a678
to
59b2607
Compare
8aa300f
to
9ec630d
Compare
The last freebsd ci failed and I can't determine exactly why. The only thing that is different is that the old code was able to pass ci and the ci image version was |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for splitting up the commit, makes it much easier to review this way.
I'm not familiar with these options myself. But looking at the docs it seems like these options do roughly the same thing, they are just set differently on different OS? Is that correct?
If so, I think we should try and merge the functions such that the user doesn't need a bunch of cfg
to make the correct call depending on the OS.
As the title says, we should not merge these behaviors, some options themselves are not the meaning of transparent proxy, they are necessary settings to complete transparent proxy, but not the only settings. UPD: |
@Thomasdezeeuw Or we should merge (set_)freebind/(set_)ip_bindany(v4|v6)/(set)so_bindany, which should be equivalent options on different platforms. For Linux, we only keep (set)ip_transparent(v4|v6). WDYT? UDP: if so, which one should we take? #[cfg(all(feature = "all", any(target_os = "android", target_os = "fuchsia", target_os = "linux")))]
pub fn set_ip_bindany_v4() -> io::Result<()> { /*IP_FREEBIND*/ }
#[cfg(all(feature = "all", target_os = "freebsd"))]
pub fn set_ip_bindany_v4() -> io::Result<()> { /*IP_BINDANY*/ }
#[cfg(all(feature = "all", target_os = "openbsd"))]
pub fn set_ip_bindany_v4() -> io::Result<()> { /*SO_BINDANY*/ } or #[cfg(all(feature = "all", any(target_os = "android", target_os = "fuchsia", target_os = "linux", target_os = "freebsd", target_os = "openbsd")))]
pub fn set_ip_bindany_v4() -> io::Result<()> {
#[cfg(any(target_os = "android", target_os = "fuchsia", target_os = "linux"))]
unsafe { /*IP_FREEBIND*/ }
#[cfg(target_os = "freebsd")]
unsafe { /*IP_BINDANY*/ }
#[cfg(target_os = "openbsd")]
unsafe { /*SO_BINDANY*/ }
} |
I prefer the second option with proper document as to what options are being set, iff the options are actually the same. We have something similar for the keep alive options. |
Now coded according to the new requirements. All changes can be seen in the PR description detail. |
Changes: