Re: [RFC iproute2] tos: interpret ToS in natural numeral system
flat view
From: David Ahern <hidden>
Date: 2022-02-17 01:52:47
On 2/16/22 3:23 PM, Guillaume Nault wrote:
On Wed, Feb 16, 2022 at 11:42:05AM -0800, Jakub Kicinski wrote:quoted
Silently forcing a base numeral system is very painful for users. ip currently interprets tos 10 as 0x10. Imagine user's bash script does: .. tos $((TOS * 2)) .. or any numerical operation on the ToS. This patch breaks existing scripts if they expect 10 to be 0x10.I agree that we shouldn't have forced base 16 in the first place. But after so many years I find it a bit dangerous to change that.
I agree. In this case the change in behavior will not be very obvious and could lead to confusion. I think this is something we have to live with.
quoted hunk ↗ jump to hunk
What about just printing a warning when the value isn't prefixed with '0x'? Something like (completely untested):@@ -535,6 +535,12 @@ int rtnl_dsfield_a2n(__u32 *id, const char *arg) if (!end || end == arg || *end || res > 255) return -1; *id = res; + + if (strncmp("0x", arg, 2)) + fprintf(stderr, + "Warning: dsfield and tos parameters are interpreted as hexadecimal values\n" + "Use 'dsfield 0x%02x' to avoid this message\n", res); + return 0; }
That seems reasonable to me to let users know of this behavior.
quoted
Signed-off-by: Jakub Kicinski <kuba@kernel.org> -- I get the feeling this was discussed in the past. Also there's more: devlink/devlink.c: val = strtoull(str, &endptr, 10); devlink/devlink.c: val = strtoul(str, &endptr, 10); devlink/devlink.c: val = strtoul(str, &endptr, 10); devlink/devlink.c: val = strtoul(str, &endptr, 10);
I think the devlink ones all check out. I think those functions can be updated to use get_uNN from lib/utils.c.
quoted
lib/utils.c: res = strtoul(arg, &ptr, base);
This is get_unsigned and those users outside of tc look ok.
quoted
lib/utils.c: n = strtoul(cp, &endp, 16);
ipv6 address conversion which is hex based.
quoted
lib/utils.c: tmp = strtoul(tmpstr, &endptr, 16); lib/utils.c: tmp = strtoul(arg + i * 3, &endptr, 16);
both of these are valid hex conversions
quoted
misc/lnstat_util.c: unsigned long f = strtoul(ptr, &ptr, 16); tc/f_u32.c: htid = strtoul(str, &tmp, 16); tc/f_u32.c: hash = strtoul(str, &tmp, 16); tc/f_u32.c: nodeid = strtoul(str, &tmp, 16); tc/tc_util.c: maj = strtoul(str, &p, 16); tc/tc_util.c: maj = strtoul(str, &p, 16); tc/tc_util.c: min = strtoul(str, &p, 16);After a very quick look, many of these seem to make sense though. For example lnstat_util.c parses output from /proc, hexstring_a2n() is used by ipmacsec.c to parse crypto keys, etc. But I agree that we should probably audit the strtol() (and variants) that don't use base 0.
Added Jamal and Cong for the tc references. Hopefully those are documented to be hex values.