refactor: Replace ToS with DSCP #2564

dgcampea · 2022-02-02T05:22:01Z

IP TOS was superseeded by DSCP in RFC 2474 (December 1998).

I've tested with wireshark and I can confirm that the DSCP options are applied (transmission-daemon).
It also passed make test.

ckerr

Looks like this is FTBFS on a few platforms, e.g. here on FreeBSD: https://trtc.mikedld.com/buildConfiguration/Transmission_Sanity_FreeBSDAmd64cyassl/170967?showRootCauses=true&expandBuildProblemsSection=true&expandBuildChangesSection=true&showLog=170967_963_628.1051

Also, please use code_style.sh (which calls clang-format) to format the code

Thanks!

dgcampea · 2022-02-02T20:01:42Z

Looks like this is FTBFS on a few platforms, e.g. here on FreeBSD: https://trtc.mikedld.com/buildConfiguration/Transmission_Sanity_FreeBSDAmd64cyassl/170967?showRootCauses=true&expandBuildProblemsSection=true&expandBuildChangesSection=true&showLog=170967_963_628.1051

I'll guess it broke on FreeBSD because <netinet/ip.h> doesn't include <netinet/in.h> though I don't have a FreeBSD machine around to readily test it. I've force-pushed a change to include both headers to test the hypothesis.

Also, please use code_style.sh (which calls clang-format) to format the code

When I ran code_style.sh it somehow mangled the code so badly that it wouldn't compile anymore. I tried clang-format manually on libtransmission/net.h and it mangles the lines after the DSCP macro definitions.

ckerr · 2022-02-02T23:17:11Z

When I ran code_style.sh it somehow mangled the code so badly that it wouldn't compile anymore. I tried clang-format manually on libtransmission/net.h and it mangles the lines after the DSCP macro definitions.

Yikes, OK don't sweat the code_style then -- different clang-format versions can give different outputs and I can re-run it after this PR gets merged. But if you are using clang-format (12, 13, or 14) and the only issue is that it's breaking macros in new code, you could try wrapping them in

// clang-format off
...
// clang-format on

ckerr · 2022-02-02T23:19:27Z

The 'Sanity' CI is still failing on the BSDs. Please LMK if you can't see the build logs when logging in as a guest to the CI box

dgcampea · 2022-02-02T23:32:36Z

Zzzz of course, I amended the commit without staging the change. Hopefully it will go through this time

dgcampea · 2022-02-02T23:36:36Z

But if you are using clang-format (12, 13, or 14) and the only issue is that it's breaking macros in new code

Using clang-format 13 here, the line it complains about is the std::map one.

IP TOS was superseeded by DSCP in RFC 2474 (December 1998).

ckerr · 2022-02-03T00:43:31Z

libtransmission/net.h

+/*
+ * Definitions for DiffServ Codepoints as per RFCs 2474, 3246, 4594 & 8622.
+ * Not all are guaranteed to be defined in <netinet/ip.h> so we keep a copy of the DSCP values.
+ */
+#ifndef IPTOS_DSCP_AF11
+#define IPTOS_DSCP_AF11  0x28
+#define IPTOS_DSCP_AF12  0x30
+#define IPTOS_DSCP_AF13  0x38
+#define IPTOS_DSCP_AF21  0x48
+#define IPTOS_DSCP_AF22  0x50
+#define IPTOS_DSCP_AF23  0x58
+#define IPTOS_DSCP_AF31  0x68
+#define IPTOS_DSCP_AF32  0x70
+#define IPTOS_DSCP_AF33  0x78
+#define IPTOS_DSCP_AF41  0x88
+#define IPTOS_DSCP_AF42  0x90
+#define IPTOS_DSCP_AF43  0x98
+#define IPTOS_DSCP_EF  0xb8
+#endif
+
+#ifndef IPTOS_DSCP_CS0
+#define IPTOS_DSCP_CS0  0x00
+#define IPTOS_DSCP_CS1  0x20
+#define IPTOS_DSCP_CS2  0x40
+#define IPTOS_DSCP_CS3  0x60
+#define IPTOS_DSCP_CS4  0x80
+#define IPTOS_DSCP_CS5  0xa0
+#define IPTOS_DSCP_CS6  0xc0
+#define IPTOS_DSCP_CS7  0xe0
+#endif
+
+#ifndef IPTOS_DSCP_EF
+#define IPTOS_DSCP_EF  0xb8
+#endif
+
+#ifndef IPTOS_DSCP_LE
+#define IPTOS_DSCP_LE  0x04
+#endif
+
+const std::map<const std::string, int> DSCPvalues {
+    { "none", INT_MAX },    // CS0 has value 0 but we want 'none' to mean OS default
+    { "",     INT_MAX },    // an alias to 'none'
+    { "af11", IPTOS_DSCP_AF11 },
+    { "af12", IPTOS_DSCP_AF12 },
+    { "af13", IPTOS_DSCP_AF13 },
+    { "af21", IPTOS_DSCP_AF21 },
+    { "af22", IPTOS_DSCP_AF22 },
+    { "af23", IPTOS_DSCP_AF23 },
+    { "af31", IPTOS_DSCP_AF31 },
+    { "af32", IPTOS_DSCP_AF32 },
+    { "af33", IPTOS_DSCP_AF33 },
+    { "af41", IPTOS_DSCP_AF41 },
+    { "af42", IPTOS_DSCP_AF42 },
+    { "af43", IPTOS_DSCP_AF43 },
+    { "cs0",  IPTOS_DSCP_CS0 },
+    { "cs1",  IPTOS_DSCP_CS1 },
+    { "cs2",  IPTOS_DSCP_CS2 },
+    { "cs3",  IPTOS_DSCP_CS3 },
+    { "cs4",  IPTOS_DSCP_CS4 },
+    { "cs5",  IPTOS_DSCP_CS5 },
+    { "cs6",  IPTOS_DSCP_CS6 },
+    { "cs7",  IPTOS_DSCP_CS7 },
+    { "ef",   IPTOS_DSCP_EF },
+    { "le",   IPTOS_DSCP_LE }


This exposes an unnecessary amount of implementation info into the header.

Maybe just declare a couple of accessor functions, e.g.

std::optional<int> tr_netDscpGetValue(std::string_view name); std::optional<std::string_view> tr_netDscpGetName(int value);

...and then move all the rest of this into net.cc

ckerr · 2022-02-03T00:45:39Z

libtransmission/net.h

+#define IPTOS_DSCP_LE  0x04
+#endif
+
+const std::map<const std::string, int> DSCPvalues {


These are all known at compile-time so there's no need to have a std::map or std::string which both require heap allocation. This should be a constexpr std::array<std::pair<std::string_view, int>, N> that we could sort at compile time and then bsearch.

Though TBH this will probably only be called a handful of times per session; linear search would also be fine

ckerr · 2022-02-03T00:49:34Z

libtransmission/net.h

+#ifndef IPTOS_DSCP_CS0
+#define IPTOS_DSCP_CS0  0x00
+#define IPTOS_DSCP_CS1  0x20
+#define IPTOS_DSCP_CS2  0x40
+#define IPTOS_DSCP_CS3  0x60
+#define IPTOS_DSCP_CS4  0x80
+#define IPTOS_DSCP_CS5  0xa0
+#define IPTOS_DSCP_CS6  0xc0
+#define IPTOS_DSCP_CS7  0xe0
+#endif


If the DSCP_AF_ and DSCP_CS values are known and never going to change, maybe just make them constexpr ints e.g. static constexpr int TR_IPTOS_DSCP_CS4 = 0x80; so that we never have to worry about header conflicts?

ckerr · 2022-02-03T00:50:10Z

libtransmission/net.h

 #include <string_view>

+#include "utils.h"


not needed in this header?

ckerr · 2022-02-03T00:50:34Z

libtransmission/net.h

@@ -9,15 +9,22 @@
 #endif

 #include <cstddef> // size_t
+#include <climits>


not needed in this header?

ckerr · 2022-02-03T00:56:10Z

libtransmission/session.cc

+    for (auto it = DSCPvalues.begin(); it != DSCPvalues.end(); ++it)
+        if (it->second == value)
+            return it->first;


consider find_if or

for (auto const& [name, val] : DSCPvalues) { if (val == value) { return name; } }

dgcampea · 2022-02-05T13:27:28Z

Hi @ckerr, thanks for the detailed review though I'm afraid I can't improve this PR much further, at least not without learning C++ the right way (you could almost say this PR was hammered together using a search engine and some rudimentary C knowledge).
If you're OK with filling or refactoring the problematic parts, I'm OK with leaving this branch open for edits (or closing it).

ckerr · 2022-02-07T01:53:28Z

OK. Please leave it up for a bit and I'll create a followup PR

ckerr · 2022-02-10T04:04:35Z

@dgcampea do you have any thoughs on #2594 ?

I've kept the same settings.json names and command-line arguments that exist in Transmission 3.0 so that existing settings won't break, but they now support the DSCP class names as well as the older ToS names, and the default for new users is a DSCP class name.

If you have feedback, let's continue this in 2594

ckerr requested changes Feb 2, 2022

View reviewed changes

dgcampea force-pushed the dscp branch from c2d4fdb to 80c2813 Compare February 2, 2022 19:52

dgcampea force-pushed the dscp branch from 80c2813 to a64351a Compare February 2, 2022 23:30

dgcampea force-pushed the dscp branch from a64351a to 2d7bf2a Compare February 2, 2022 23:41

refactor: Replace ToS with DSCP

e85703e

IP TOS was superseeded by DSCP in RFC 2474 (December 1998).

dgcampea force-pushed the dscp branch from 2d7bf2a to e85703e Compare February 3, 2022 00:47

ckerr reviewed Feb 3, 2022

View reviewed changes

Merge branch 'main' into dscp

83727b4

ckerr marked this pull request as draft February 7, 2022 01:53

ckerr mentioned this pull request Feb 10, 2022

feat: support dscp classes in socket iptos #2594

Merged

ckerr closed this Feb 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Replace ToS with DSCP #2564

refactor: Replace ToS with DSCP #2564

dgcampea commented Feb 2, 2022

ckerr left a comment •

edited

dgcampea commented Feb 2, 2022

ckerr commented Feb 2, 2022

ckerr commented Feb 2, 2022

dgcampea commented Feb 2, 2022

dgcampea commented Feb 2, 2022

ckerr Feb 3, 2022

ckerr Feb 3, 2022 •

edited

ckerr Feb 3, 2022

ckerr Feb 3, 2022

ckerr Feb 3, 2022

ckerr Feb 3, 2022

dgcampea commented Feb 5, 2022

ckerr commented Feb 7, 2022

ckerr commented Feb 10, 2022 •

edited

refactor: Replace ToS with DSCP #2564

refactor: Replace ToS with DSCP #2564

Conversation

dgcampea commented Feb 2, 2022

ckerr left a comment • edited

Choose a reason for hiding this comment

dgcampea commented Feb 2, 2022

ckerr commented Feb 2, 2022

ckerr commented Feb 2, 2022

dgcampea commented Feb 2, 2022

dgcampea commented Feb 2, 2022

ckerr Feb 3, 2022

Choose a reason for hiding this comment

ckerr Feb 3, 2022 • edited

Choose a reason for hiding this comment

ckerr Feb 3, 2022

Choose a reason for hiding this comment

ckerr Feb 3, 2022

Choose a reason for hiding this comment

ckerr Feb 3, 2022

Choose a reason for hiding this comment

ckerr Feb 3, 2022

Choose a reason for hiding this comment

dgcampea commented Feb 5, 2022

ckerr commented Feb 7, 2022

ckerr commented Feb 10, 2022 • edited

ckerr left a comment •

edited

ckerr Feb 3, 2022 •

edited

ckerr commented Feb 10, 2022 •

edited