From 0e384b017053bc513857ff03ced0917c0defd274 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 16 Sep 2021 16:50:14 +0200 Subject: [PATCH] initrd: refactor parsing of "rd.ethtool=" to not return after autoneg Don't return early from parsing "autoneg", if there are not additional arguments. The behavior should be exactly the same, whether a positional argument is missing, empty, or set to the default. That is, - "rd.ethtool=eth0:on" - "rd.ethtool=eth0:on:" - "rd.ethtool=eth0:on::" - "rd.ethtool=eth0:on:0:" should all evaluate the same thing. That was already the case in practice, but that was hard to see. So don't treat missing positional arguments special and don't return early. Parse all parameters regardless. The change is visible when parsing "rd.ethtool=eth0:off:100 rd.ethtool=eth0:on". Autoneg and speed really belongs together, so when we parse the second argument, we should reset the speed too -- even if it's not present. --- src/nm-initrd-generator/nmi-cmdline-reader.c | 56 ++++++++++--------- .../tests/test-cmdline-reader.c | 8 +-- 2 files changed, 33 insertions(+), 31 deletions(-) diff --git a/src/nm-initrd-generator/nmi-cmdline-reader.c b/src/nm-initrd-generator/nmi-cmdline-reader.c index 2afdb8dedd..367962d99d 100644 --- a/src/nm-initrd-generator/nmi-cmdline-reader.c +++ b/src/nm-initrd-generator/nmi-cmdline-reader.c @@ -1173,13 +1173,13 @@ reader_parse_rd_znet(Reader *reader, char *argument, gboolean net_ifnames) static void reader_parse_ethtool(Reader *reader, char *argument) { - const char * interface = NULL; - NMConnection * connection = NULL; - NMSettingWired *s_wired = NULL; - const char * autoneg_str = NULL; - gboolean autoneg = FALSE; - const char * speed_str = NULL; - guint speed = 0; + NMConnection * connection; + NMSettingWired *s_wired; + const char * autoneg_str; + const char * speed_str; + const char * interface; + int autoneg; + guint speed; interface = get_word(&argument, ':'); if (!interface) { @@ -1192,38 +1192,40 @@ reader_parse_ethtool(Reader *reader, char *argument) return; } - connection = reader_get_connection(reader, interface, NM_SETTING_WIRED_SETTING_NAME, TRUE); - s_wired = nm_connection_get_setting_wired(connection); - autoneg_str = get_word(&argument, ':'); + speed_str = get_word(&argument, ':'); + + autoneg = -1; if (autoneg_str) { autoneg = _nm_utils_ascii_str_to_bool(autoneg_str, -1); if (autoneg == -1) _LOGW(LOGD_CORE, "Invalid value for rd.ethtool.autoneg, rd.ethtool.autoneg was not set"); - else - g_object_set(s_wired, NM_SETTING_WIRED_AUTO_NEGOTIATE, autoneg, NULL); } - if (!*argument) - return; - speed_str = get_word(&argument, ':'); + speed = 0; if (speed_str) { - speed = _nm_utils_ascii_str_to_int64(speed_str, 10, 0, G_MAXUINT32, -1); - if (speed == -1) + speed = _nm_utils_ascii_str_to_int64(speed_str, 10, 0, G_MAXUINT32, 0); + if (errno) _LOGW(LOGD_CORE, "Invalid value for rd.ethtool.speed, rd.ethtool.speed was not set"); - else - g_object_set(s_wired, - NM_SETTING_WIRED_SPEED, - speed, - NM_SETTING_WIRED_DUPLEX, - "full", - NULL); } - if (!*argument) - return; - else + if (autoneg == -1) + autoneg = FALSE; + + connection = reader_get_connection(reader, interface, NM_SETTING_WIRED_SETTING_NAME, TRUE); + s_wired = nm_connection_get_setting_wired(connection); + + g_object_set(s_wired, + NM_SETTING_WIRED_AUTO_NEGOTIATE, + (gboolean) autoneg, + NM_SETTING_WIRED_SPEED, + speed, + NM_SETTING_WIRED_DUPLEX, + speed == 0 ? NULL : "full", + NULL); + + if (*argument) _LOGW(LOGD_CORE, "Invalid extra argument '%s' for rd.ethtool, this value was not set", argument); diff --git a/src/nm-initrd-generator/tests/test-cmdline-reader.c b/src/nm-initrd-generator/tests/test-cmdline-reader.c index f2ad5470ff..ccdb8ab7be 100644 --- a/src/nm-initrd-generator/tests/test-cmdline-reader.c +++ b/src/nm-initrd-generator/tests/test-cmdline-reader.c @@ -2403,10 +2403,10 @@ test_rd_ethtool(void) _ethtool_check_v(NM_MAKE_STRV("rd.ethtool=eth0:off:0", "rd.ethtool=eth0:off"), FALSE, 0); _ethtool_check_v(NM_MAKE_STRV("rd.ethtool=eth0:on:0", "rd.ethtool=eth0:on"), TRUE, 0); _ethtool_check_v(NM_MAKE_STRV("rd.ethtool=eth0:on:0", "rd.ethtool=eth0:off"), FALSE, 0); - _ethtool_check_v(NM_MAKE_STRV("rd.ethtool=eth0:off:100", "rd.ethtool=eth0:on"), TRUE, 100); - _ethtool_check_v(NM_MAKE_STRV("rd.ethtool=eth0:off:100", "rd.ethtool=eth0:off"), FALSE, 100); - _ethtool_check_v(NM_MAKE_STRV("rd.ethtool=eth0:on:100", "rd.ethtool=eth0:on"), TRUE, 100); - _ethtool_check_v(NM_MAKE_STRV("rd.ethtool=eth0:on:100", "rd.ethtool=eth0:off"), FALSE, 100); + _ethtool_check_v(NM_MAKE_STRV("rd.ethtool=eth0:off:100", "rd.ethtool=eth0:on"), TRUE, 0); + _ethtool_check_v(NM_MAKE_STRV("rd.ethtool=eth0:off:100", "rd.ethtool=eth0:off"), FALSE, 0); + _ethtool_check_v(NM_MAKE_STRV("rd.ethtool=eth0:on:100", "rd.ethtool=eth0:on"), TRUE, 0); + _ethtool_check_v(NM_MAKE_STRV("rd.ethtool=eth0:on:100", "rd.ethtool=eth0:off"), FALSE, 0); NMTST_EXPECT_NM_WARN("cmdline-reader: Could not find rd.ethtool options to set"); _ethtool_check_v(NM_MAKE_STRV("rd.ethtool=eth0:off:100", "rd.ethtool=eth0:"), FALSE, 100);