-
-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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 IP Type of Service #13606
base: master
Are you sure you want to change the base?
Support IP Type of Service #13606
Conversation
ddc3935
to
2c5db47
Compare
I don't know what to do with the remaining tests that fail. Can you please advise? |
Why is a new libcurl option needed? Can't |
This is a IPv4-only feature. What happens for IPv6 sockets and how is a user supposed to know when this works or not? Can you please explain your use case for this? |
We use it with the command-line interface. And if I understand correctly, |
We need it for applying QoS for IP packets. All the other parts of our application support this, and we've been patching curl for long time to add support. Now we'd like to upstream our patches (2 more will follow :)). I noticed that another used proposed a similar change in #7762, but it was abandoned. I took the user-friendly names from that PR. |
You add |
It is used in |
bcb0e25
to
1af883c
Compare
I do. I don't see why libcurl needs another option when you can already accomplish the same thing using an existing option. |
The next feature I plan to add is setting VLAN priority, which is another setsockopt ( |
So do both in the same callback? |
dce54bd
to
e6a7c46
Compare
Done. Is this what you meant? |
c290eb0
to
0d692fa
Compare
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'm also interested in this feature, so thanks for writing it up!
0cfe17e
to
23ece88
Compare
@bagder Is this good to go, now that 8.8 was released? |
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.
We cannot merge this anyway until the feature window opens on June 1st if everything goes well.
6214d8d
to
1442688
Compare
Is the msys2 with debug flaky? I couldn't find anything in the log that looks related to my changes. |
@orgads: Yes, the MSYS2 mingw-w64 ones sometimes hang in tests. I'm tracking the problem here: #13599 (comment). Could not yet figure out which test is prone to hanging though. |
docs/cmdline-opts/type-of-service.md
Outdated
|
||
# `--type-of-service` | ||
|
||
Set IP Type of Service (TOS). (Added in 8.9.0). |
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.
Since you added support for IPv6 and its "Traffic Class" field:
- Should this option really be named like this now?
- Should the documentation not specify the IPv6 details?
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 have a better suggestion? Maybe add another
--traffic-class
which does the same? - These fields have the same purpose and structure (6 bits DSCP, 2 bits ECN), only the name and the location within the packet differs.
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.
No, this should be a single option I think. Having two different ones doing the same thing does not make anything easier for anyone. It would make more sense to come up with a generic name that works for applying to both cases. Maybe it should even include "IP" in the name to better hint what it is about.
Maybe...
--ip-traffic
--ip-type
--ip-service
?
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'm not sure any of these suggestions is more descriptive than type-of-service.
TOS used to have a different meaning, but nowadays, it is just like traffic class, divided to 6 bits DSCP and 2 bits ECN.
Maybe we should just have --ip-dscp
and --ip-ecn
?
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.
Naming discussions!🎉
How about --ip-tos
which should be descriptive for people who know what it is and yet short.
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.
Just some helpful info on other tools:
linux iproute2 has tos
and its alias dsfield
for adding/showing routes that match on the dsfield. It accepts a named DSCP value or a masked 8 bit TOS value, and the ECN bits are ignored. The option is named the same for both v4 and v6 routing tables. E.g. ip route add to 1.2.3.4/32 dsfield AF21 counter
linux nftables has ip dscp
and ip6 dscp
payload expresions that match/set the 6 bit ds field in the respective protocol family. It accepts either a named or 6 bit DSCP value. E.g. nft add rule inet filter input ip dscp af21 meta priority set 1:4
.
tcpdump -v
displays the tos / tclass byte as "tos" or "class" followed by the 8 bit hex value of the byte, including the ECN field.
ping -Q
sets the tos / tclass byte just like this patch, and linux copies the entire byte in echo replies.
--ip-tos
or even just --tos
seems fine to me. I'm partial to --dscp
I think. I mentioned it earlier but linux ignores the ECN field from this sockopt for SOCK_STREAM sockets, and I suppose the quic library will manage the ECN field otherwise, so I expect that setting the the ECN field with this option will never be effective, hence --dscp
seems reasonable.
Is there a way to retrigger only the failing job? |
Passed, thanks! |
Adds a
--type-of-service
option the command line tool for setting the TOS IPv4 header field.