-
Notifications
You must be signed in to change notification settings - Fork 240
IPv6 QoS fix #3622
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
base: main
Are you sure you want to change the base?
IPv6 QoS fix #3622
Conversation
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've been doing some tests of this on Linux (RPi), and it needs a little more investigation.
I verified that the setsockopt() previously returned an error, and that with this fix it succeeds.
I also tested it with different QoS values for server and client, running on the same machine connecting via localhost (::1) on IPv6, while monitoring the traffic with Wireshark. The correct QoS (DSCP+ECN) was sent from each end.
However, if the client initiated the connection to 127.0.0.1 instead, to force an IPv4 connection, both ends sent a QoS value of 0.
If I started the client without -6, and connected to 127.0.0.1 or to localhost, then the traffic from the client to the server carried the expected QoS value, but that from the server (still running with -6, so a dual-protocol socket) back to the client sent QoS of 0.
So I need to check how to get a dual-stack socket to send the correct QoS value when it has an IPv4 connection. Maybe it will also accept the setsockopt ( UdpSocket, IPPROTO_IP, IP_TOS, ...) call?
Yes, it works. I'll update the PR directly. |
|
I have done some builds on Windows. Here are my findings:
Looking at https://learn.microsoft.com/en-us/previous-versions/windows/desktop/qos/sending-qos-enabled-data it would appear that setting QoS in Windows uses a completely different API from that on Linux. For 3.12.0, I would suggest that we just specify QoS is not supported on Windows. We may want to reject It would be worth checking on Mac too, but hopefully that should be ok, as Mac is Unix-based. |
|
I vaguely remember a long and detailed explanation of what was needed to explain to Windows (10 at the time, IIRC) that QoS/TOS should be respected. This was either on the original PR or in the issue or discussion it was related to. It's probably worth digging that up and reviewing it. |
Sadly, it doesn't work completely on macOS. Using ChatGPT confirmed this is the case, and suggested the best way round it would be to create a drop-in replacement for |
|
Well so far, I have #if-ed out the calls that will error on Windows and Apple. With this change, the following are true:
|
|
Gemini says for Windows (though I didn't check versions):
Sounds like it's very version specific but originated around Windows Vista/7 and was fully supported in Windows 10 onwards. (Not Windows Server, though.) It did also reference the "hack" I vaguely remembered:
And then "Socket Option: IPPROTO_IP level, option IP_TOS", so long as Group Policy allows it. I think I went with group policy:
I'd imagine the NSIS installer has a Group Policy hook? Maybe it could be done there. |
|
And it found the original discussion for me: https://github.com/orgs/jamulussoftware/discussions/1072 The original discussion makes a good point: the application itself should know the right value -- it shouldn't be the user picking numbers at random from the command line. |
|
Interesting stuff. So what to do with this PR, and what to defer to another? This one has already improved a bit on @rdica's original, without much radical change, and now fully works on Linux. It seems to me that this is worth merging now for 3.12.0, and then we raise two more separate PRs for after 3.12.0:
Comments? |
Yes, agreed. Although if It would be worth understanding which other values are useful for Jamulus, and providing useful names for the user to specify them. Again, post-3.12 |
Short description of changes
When IPv6 is enabled, this provides a fix to properly typecast
tosso setsockopt can properly add DSCP field value.Also adds error checking for
setsockopt()calls and ensures correct QoS operation for dual-protocol sockets.CHANGELOG: Client+Server: Correct QoS setting for IPv6
Context: Fixes an issue?
Fixes: #3623
Does this change need documentation? What needs to be documented and how?
No, just a bugfix
Status of this Pull Request
What is missing until this pull request can be merged?
Needs testing on all but Linux OSes.
Checklist