-
-
Notifications
You must be signed in to change notification settings - Fork 485
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
Feature: support setSocketOptions on windows #192
Conversation
It seems mistake that base on |
Hi, thanks for the PR! We don’t need For example, in Darwin: tun2socks/component/dialer/sockopt_darwin.go Lines 29 to 34 in 3cbc74b
|
Hi, @xjasonlyu pc, err := dialer.ListenPacketWithOptions("udp", "", &dialer.Options{
InterfaceName: mylocalName,
InterfaceIndex: mylocalIndex,
})
if err != nil {
// to do err
}
uAddr, _ := net.ResolveUDPAddr("udp", "114.114.114.114:53")
pc.WriteTo(mydata, uAddr) Based on the code above, I found the There are two ways to fix the routing loop issue that I found:
I'm confused with the result. Any thought??
|
I guess it might have something to do with Windows' long ipv6 representing ipv4 addresses. |
Hi, @xjasonlyu please review the code again. |
I like this PR since I wondered how to solve routing loop issue on Windows before. |
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.
Hi, please see the reviews.
@xjasonlyu Hi, replied to you. Please take a look. |
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.
Thoughts here.
Also, this feature is only supported on Windows 11, right? |
From the doc(https://learn.microsoft.com/en-us/windows/win32/winsock/ipproto-ip-socket-options#windows-support-for-ip_proto-options), it doesn't support windows10 and lower. But I want to do a test on windows10 in Virtual Machine, I'll give you the result later. |
component/dialer/sockopt_windows.go
Outdated
} | ||
|
||
func bindSocketToInterface4(handle windows.Handle, interfaceIndex uint32) error { | ||
const IP_UNICAST_IF = 31 |
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.
Do you think we should put the const within the func or declare it globally?
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.
It seems more reasonable that move IP_UNICAST_IF
to global.
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.
And how about IP_UNICAST_IF6
? They're different param, right?
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.
I think we can use IP_UNICAST_IF
for all of them, since the value is the same. I've done some research, I found the IP_UNICAST_IF
param is unique. Here is the windows.rs example: https://microsoft.github.io/windows-docs-rs/doc/windows/Win32/Networking/WinSock/constant.IP_UNICAST_IF.html I didn't find IP_UNICAST_IF6
or similar.
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.
There is a param for IPv6: IPV6_UNICAST_IF
, see https://learn.microsoft.com/en-us/windows/win32/winsock/ipproto-ipv6-socket-options, nut I wonder if they have the same value.
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.
Oh sorry, It is true that IPV6_UNICAST_IF
is for IPV6. I also find it in windows.rs and the value is 31
as well. I've updated the code.
Ok, thanks. And let me know what happens actually, so we can discuss how to handle those circumstances. |
I did the test on windows10 in Virtual Machine. I found it works fine on windows10 as well. |
That's weird but OK. |
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.
Hi, making last style changes, and I think it's good for merge.
Hi, the style changes have been done. |
Great, thank you for the PR and all the hard effort! |
Hi, just to mention, someone has tested this feature under Windows 10 21H2 or below, but it doesn't seem to work. |
That's weird. Could him open a issue? I want to know how to reproduce it. |
I've had it tested, there's no problem. It turns out he misconfigured the interface name. 🤦♂️ |
很重要的功能 以及 对于 ipv4部分的 endian转换的发现!感谢 |
Hi, I have made a PR for supporting setSocketOptions on windows, since it is not supported now.
The idea is taken from wireguard-go