ifcfg-rh: merge branch 'th/ethtool-ifcfg-fixes'

https://bugzilla.redhat.com/show_bug.cgi?id=2134569

https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1452

8 files changed, 236 insertions, 1 deletions

diff --git a/Makefile.am b/Makefile.am
index 8e60a5f5bd..f0fa1fb760 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -3665,6 +3665,10 @@ EXTRA_DIST += \
 	src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test_roundtrip_ethtool-3.cexpected \
 	src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test_roundtrip_ethtool-4.cexpected \
 	src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test_roundtrip_ethtool-5.cexpected \
+	src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test_roundtrip_ethtool-6.cexpected \
+	src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test_roundtrip_ethtool-7.cexpected \
+	src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test_roundtrip_ethtool-8.cexpected \
+	src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test_roundtrip_ethtool-9.cexpected \
 	src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test_write_wired_auto_negotiate_on.cexpected \
 	src/core/settings/plugins/ifcfg-rh/tests/network-scripts/keys-test-wifi-dynamic-wep-leap \
 	src/core/settings/plugins/ifcfg-rh/tests/network-scripts/keys-test-wifi-leap \
diff --git a/src/core/devices/nm-device-ethernet.c b/src/core/devices/nm-device-ethernet.c
index 414bfa7b4b..9852d659ee 100644
--- a/src/core/devices/nm-device-ethernet.c
+++ b/src/core/devices/nm-device-ethernet.c
@@ -1354,6 +1354,11 @@ wake_on_lan_enable(NMDevice *device)
     if (s_wired) {
         wol      = nm_setting_wired_get_wake_on_lan(s_wired);
         password = nm_setting_wired_get_wake_on_lan_password(s_wired);
+
+        /* NMSettingWired does not reject invalid flags. Filter them out here. */
+        wol = (wol
+               & (NM_SETTING_WIRED_WAKE_ON_LAN_ALL | NM_SETTING_WIRED_WAKE_ON_LAN_EXCLUSIVE_FLAGS));
+
         if (wol != NM_SETTING_WIRED_WAKE_ON_LAN_DEFAULT)
             goto found;
     }
@@ -1370,9 +1375,14 @@ wake_on_lan_enable(NMDevice *device)
         nm_log_dbg(LOGD_ETHER, "invalid default value %u for wake-on-lan", (guint) wol);
         wol = NM_SETTING_WIRED_WAKE_ON_LAN_DEFAULT;
     }
+
+    wol = wol & (NM_SETTING_WIRED_WAKE_ON_LAN_ALL | NM_SETTING_WIRED_WAKE_ON_LAN_EXCLUSIVE_FLAGS);
+
     if (wol != NM_SETTING_WIRED_WAKE_ON_LAN_DEFAULT)
         goto found;
+
     wol = NM_SETTING_WIRED_WAKE_ON_LAN_IGNORE;
+
 found:
     return nm_platform_ethtool_set_wake_on_lan(nm_device_get_platform(device),
                                                nm_device_get_ifindex(device),
diff --git a/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c b/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c
index d8c6a839f0..673ee1c287 100644
--- a/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c
+++ b/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c
@@ -1312,6 +1312,7 @@ write_ethtool_setting(NMConnection *connection, shvarFile *ifcfg, GError **error
         gboolean             is_first;
         guint32              u32;
         gboolean             b;
+        gboolean             any_option = FALSE;
 
         s_con = nm_connection_get_setting_connection(connection);
         if (s_con) {
@@ -1341,6 +1342,7 @@ write_ethtool_setting(NMConnection *connection, shvarFile *ifcfg, GError **error
             g_string_append_c(str, ' ');
             g_string_append(str, nms_ifcfg_rh_utils_get_ethtool_name(ethtool_id));
             g_string_append(str, b ? " on" : " off");
+            any_option = TRUE;
         }
 
         is_first = TRUE;
@@ -1356,6 +1358,7 @@ write_ethtool_setting(NMConnection *connection, shvarFile *ifcfg, GError **error
             g_string_append_c(str, ' ');
             g_string_append(str, nms_ifcfg_rh_utils_get_ethtool_name(ethtool_id));
             g_string_append_printf(str, " %" G_GUINT32_FORMAT, u32);
+            any_option = TRUE;
         }
 
         is_first = TRUE;
@@ -1371,7 +1374,10 @@ write_ethtool_setting(NMConnection *connection, shvarFile *ifcfg, GError **error
             g_string_append_c(str, ' ');
             g_string_append(str, nms_ifcfg_rh_utils_get_ethtool_name(ethtool_id));
             g_string_append_printf(str, " %" G_GUINT32_FORMAT, u32);
+            any_option = TRUE;
         }
+
+        is_first = TRUE;
         for (ethtool_id = _NM_ETHTOOL_ID_PAUSE_FIRST; ethtool_id <= _NM_ETHTOOL_ID_PAUSE_LAST;
              ethtool_id++) {
             nm_assert(nms_ifcfg_rh_utils_get_ethtool_name(ethtool_id));
@@ -1384,9 +1390,10 @@ write_ethtool_setting(NMConnection *connection, shvarFile *ifcfg, GError **error
             g_string_append_c(str, ' ');
             g_string_append(str, nms_ifcfg_rh_utils_get_ethtool_name(ethtool_id));
             g_string_append(str, b ? " on" : " off");
+            any_option = TRUE;
         }
 
-        if (!str) {
+        if (!any_option) {
             /* Write an empty dummy "-A" option without arguments. This is to
              * ensure that the reader will create an (all default) NMSettingEthtool.
              * Also, it seems that `ethtool -A "$IFACE"` is silently accepted. */
diff --git a/src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test_roundtrip_ethtool-6.cexpected b/src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test_roundtrip_ethtool-6.cexpected
new file mode 100644
index 0000000000..784ad22427
--- /dev/null
+++ b/src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test_roundtrip_ethtool-6.cexpected
@@ -0,0 +1,15 @@
+TYPE=Ethernet
+PROXY_METHOD=none
+BROWSER_ONLY=no
+ETHTOOL_OPTS="autoneg on ; -A net0 pause-autoneg off"
+BOOTPROTO=dhcp
+DEFROUTE=yes
+IPV4_FAILURE_FATAL=no
+IPV6INIT=yes
+IPV6_AUTOCONF=yes
+IPV6_DEFROUTE=yes
+IPV6_FAILURE_FATAL=no
+IPV6_ADDR_GEN_MODE=default
+NAME=test_roundtrip_ethtool
+UUID=${UUID}
+ONBOOT=yes
diff --git a/src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test_roundtrip_ethtool-7.cexpected b/src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test_roundtrip_ethtool-7.cexpected
new file mode 100644
index 0000000000..02ab14fee6
--- /dev/null
+++ b/src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test_roundtrip_ethtool-7.cexpected
@@ -0,0 +1,15 @@
+TYPE=Ethernet
+PROXY_METHOD=none
+BROWSER_ONLY=no
+ETHTOOL_OPTS="autoneg on ; -G net0 rx 512"
+BOOTPROTO=dhcp
+DEFROUTE=yes
+IPV4_FAILURE_FATAL=no
+IPV6INIT=yes
+IPV6_AUTOCONF=yes
+IPV6_DEFROUTE=yes
+IPV6_FAILURE_FATAL=no
+IPV6_ADDR_GEN_MODE=default
+NAME=test_roundtrip_ethtool
+UUID=${UUID}
+ONBOOT=yes
diff --git a/src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test_roundtrip_ethtool-8.cexpected b/src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test_roundtrip_ethtool-8.cexpected
new file mode 100644
index 0000000000..fd76083936
--- /dev/null
+++ b/src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test_roundtrip_ethtool-8.cexpected
@@ -0,0 +1,15 @@
+TYPE=Ethernet
+PROXY_METHOD=none
+BROWSER_ONLY=no
+ETHTOOL_OPTS="autoneg on ; -A net0"
+BOOTPROTO=dhcp
+DEFROUTE=yes
+IPV4_FAILURE_FATAL=no
+IPV6INIT=yes
+IPV6_AUTOCONF=yes
+IPV6_DEFROUTE=yes
+IPV6_FAILURE_FATAL=no
+IPV6_ADDR_GEN_MODE=default
+NAME=test_roundtrip_ethtool
+UUID=${UUID}
+ONBOOT=yes
diff --git a/src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test_roundtrip_ethtool-9.cexpected b/src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test_roundtrip_ethtool-9.cexpected
new file mode 100644
index 0000000000..1703b0dc77
--- /dev/null
+++ b/src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test_roundtrip_ethtool-9.cexpected
@@ -0,0 +1,15 @@
+TYPE=Ethernet
+PROXY_METHOD=none
+BROWSER_ONLY=no
+ETHTOOL_OPTS="autoneg on ; -G net0 rx 512 ; -A net0 pause-autoneg off"
+BOOTPROTO=dhcp
+DEFROUTE=yes
+IPV4_FAILURE_FATAL=no
+IPV6INIT=yes
+IPV6_AUTOCONF=yes
+IPV6_DEFROUTE=yes
+IPV6_FAILURE_FATAL=no
+IPV6_ADDR_GEN_MODE=default
+NAME=test_roundtrip_ethtool
+UUID=${UUID}
+ONBOOT=yes
diff --git a/src/core/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c b/src/core/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c
index aba9b922dc..108c6ccd57 100644
--- a/src/core/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c
+++ b/src/core/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c
@@ -3438,6 +3438,7 @@ test_roundtrip_ethtool(void)
     gs_unref_object NMConnection *connection = NULL;
     NMSetting                    *s_ethtool;
     NMSetting                    *s_wired;
+    int                           i_run;
 
     connection = nmtst_create_minimal_connection("test_roundtrip_ethtool",
                                                  NULL,
@@ -3500,6 +3501,159 @@ test_roundtrip_ethtool(void)
                            TEST_IFCFG_DIR "/ifcfg-test_roundtrip_ethtool-5.cexpected",
                            NULL);
     g_clear_object(&connection);
+
+    connection = nmtst_create_minimal_connection("test_roundtrip_ethtool",
+                                                 NULL,
+                                                 NM_SETTING_WIRED_SETTING_NAME,
+                                                 NULL);
+    s_wired    = nmtst_connection_assert_setting(connection, NM_TYPE_SETTING_WIRED);
+    g_object_set(s_wired, NM_SETTING_WIRED_AUTO_NEGOTIATE, TRUE, NULL);
+
+    s_ethtool = _nm_connection_new_setting(connection, NM_TYPE_SETTING_ETHTOOL);
+    nm_setting_option_set_boolean(s_ethtool, NM_ETHTOOL_OPTNAME_PAUSE_AUTONEG, FALSE);
+    _writer_new_connec_exp(connection,
+                           TEST_SCRATCH_DIR,
+                           TEST_IFCFG_DIR "/ifcfg-test_roundtrip_ethtool-6.cexpected",
+                           NULL);
+    g_clear_object(&connection);
+
+    connection = nmtst_create_minimal_connection("test_roundtrip_ethtool",
+                                                 NULL,
+                                                 NM_SETTING_WIRED_SETTING_NAME,
+                                                 NULL);
+    s_wired    = nmtst_connection_assert_setting(connection, NM_TYPE_SETTING_WIRED);
+    g_object_set(s_wired, NM_SETTING_WIRED_AUTO_NEGOTIATE, TRUE, NULL);
+
+    s_ethtool = _nm_connection_new_setting(connection, NM_TYPE_SETTING_ETHTOOL);
+    nm_setting_option_set_uint32(s_ethtool, NM_ETHTOOL_OPTNAME_RING_RX, 512);
+    _writer_new_connec_exp(connection,
+                           TEST_SCRATCH_DIR,
+                           TEST_IFCFG_DIR "/ifcfg-test_roundtrip_ethtool-7.cexpected",
+                           NULL);
+    g_clear_object(&connection);
+
+    connection = nmtst_create_minimal_connection("test_roundtrip_ethtool",
+                                                 NULL,
+                                                 NM_SETTING_WIRED_SETTING_NAME,
+                                                 NULL);
+    s_wired    = nmtst_connection_assert_setting(connection, NM_TYPE_SETTING_WIRED);
+    g_object_set(s_wired, NM_SETTING_WIRED_AUTO_NEGOTIATE, TRUE, NULL);
+
+    s_ethtool = _nm_connection_new_setting(connection, NM_TYPE_SETTING_ETHTOOL);
+    _writer_new_connec_exp(connection,
+                           TEST_SCRATCH_DIR,
+                           TEST_IFCFG_DIR "/ifcfg-test_roundtrip_ethtool-8.cexpected",
+                           NULL);
+    g_clear_object(&connection);
+
+    connection = nmtst_create_minimal_connection("test_roundtrip_ethtool",
+                                                 NULL,
+                                                 NM_SETTING_WIRED_SETTING_NAME,
+                                                 NULL);
+    s_wired    = nmtst_connection_assert_setting(connection, NM_TYPE_SETTING_WIRED);
+    g_object_set(s_wired, NM_SETTING_WIRED_AUTO_NEGOTIATE, TRUE, NULL);
+
+    s_ethtool = _nm_connection_new_setting(connection, NM_TYPE_SETTING_ETHTOOL);
+    nm_setting_option_set_boolean(s_ethtool, NM_ETHTOOL_OPTNAME_PAUSE_AUTONEG, FALSE);
+    nm_setting_option_set_uint32(s_ethtool, NM_ETHTOOL_OPTNAME_RING_RX, 512);
+    _writer_new_connec_exp(connection,
+                           TEST_SCRATCH_DIR,
+                           TEST_IFCFG_DIR "/ifcfg-test_roundtrip_ethtool-9.cexpected",
+                           NULL);
+    g_clear_object(&connection);
+
+    for (i_run = 0; i_run < 20; i_run++) {
+        gs_unref_object NMConnection *con2     = NULL;
+        gs_unref_object NMConnection *reread   = NULL;
+        nmtst_auto_unlinkfile char   *testfile = NULL;
+        gboolean                      reread_same;
+        gboolean                      v_bool;
+        NMEthtoolID                   ethtool_ids[_NM_ETHTOOL_ID_NUM];
+        guint                         i;
+        guint                         l;
+        NMSettingWiredWakeOnLan       wake_on_lan;
+
+        con2    = nmtst_create_minimal_connection("test_roundtrip_ethtool",
+                                               NULL,
+                                               NM_SETTING_WIRED_SETTING_NAME,
+                                               NULL);
+        s_wired = nmtst_connection_assert_setting(con2, NM_TYPE_SETTING_WIRED);
+        g_object_set(s_wired, NM_SETTING_WIRED_AUTO_NEGOTIATE, nmtst_get_rand_bool(), NULL);
+
+        if (nmtst_get_rand_bool()) {
+            g_object_set(s_wired,
+                         NM_SETTING_WIRED_SPEED,
+                         1000u,
+                         NM_SETTING_WIRED_DUPLEX,
+                         nmtst_get_rand_bool() ? "full" : "half",
+                         NULL);
+        }
+
+        wake_on_lan = nmtst_get_rand_uint32();
+        wake_on_lan = wake_on_lan & NM_SETTING_WIRED_WAKE_ON_LAN_ALL;
+        wake_on_lan = nmtst_rand_select(wake_on_lan,
+                                        NM_SETTING_WIRED_WAKE_ON_LAN_DEFAULT,
+                                        NM_SETTING_WIRED_WAKE_ON_LAN_IGNORE);
+        g_object_set(s_wired, NM_SETTING_WIRED_WAKE_ON_LAN, (guint) wake_on_lan, NULL);
+
+        if (NM_FLAGS_HAS(wake_on_lan, NM_SETTING_WIRED_WAKE_ON_LAN_MAGIC)
+            && nmtst_get_rand_bool()) {
+            g_object_set(s_wired, NM_SETTING_WIRED_WAKE_ON_LAN_PASSWORD, "aa:bb:cc:dd:ee:ff", NULL);
+        }
+
+        l = nmtst_get_rand_uint32() % (G_N_ELEMENTS(ethtool_ids) + 2);
+        if (l == 0) {
+            /* pass. No ethtool setting. */
+            goto check_roundtrip;
+        }
+        l--;
+
+        s_ethtool = _nm_connection_new_setting(con2, NM_TYPE_SETTING_ETHTOOL);
+
+        for (i = 0; i < (int) G_N_ELEMENTS(ethtool_ids); i++)
+            ethtool_ids[i] = i;
+        nmtst_rand_perm(NULL, ethtool_ids, NULL, sizeof(ethtool_ids[0]), G_N_ELEMENTS(ethtool_ids));
+
+        for (i = 0; i < l; i++) {
+            NMEthtoolID         ethtool_id = ethtool_ids[i];
+            const GVariantType *vtype;
+            const char         *optname;
+
+            optname = nm_ethtool_data[ethtool_id]->optname;
+            vtype   = nm_ethtool_id_get_variant_type(ethtool_id);
+
+            if (NM_IN_SET(ethtool_id,
+                          NM_ETHTOOL_ID_COALESCE_ADAPTIVE_RX,
+                          NM_ETHTOOL_ID_COALESCE_ADAPTIVE_TX)) {
+                nm_setting_option_set_uint32(s_ethtool, optname, nmtst_get_rand_uint32() % 2);
+            } else if (g_variant_type_equal(vtype, G_VARIANT_TYPE_BOOLEAN))
+                nm_setting_option_set_boolean(s_ethtool, optname, nmtst_get_rand_bool());
+            else if (g_variant_type_equal(vtype, G_VARIANT_TYPE_UINT32))
+                nm_setting_option_set_uint32(s_ethtool, optname, nmtst_get_rand_uint32());
+            else
+                g_assert_not_reached();
+        }
+
+        if ((nm_setting_option_get_boolean(s_ethtool, NM_ETHTOOL_OPTNAME_PAUSE_RX, NULL)
+             || nm_setting_option_get_boolean(s_ethtool, NM_ETHTOOL_OPTNAME_PAUSE_TX, NULL))
+            && nm_setting_option_get_boolean(s_ethtool, NM_ETHTOOL_OPTNAME_PAUSE_AUTONEG, &v_bool)
+            && v_bool) {
+            /* don't accidentally create an invalid profile. */
+            nm_setting_option_set(s_ethtool,
+                                  NM_ETHTOOL_OPTNAME_PAUSE_AUTONEG,
+                                  nmtst_get_rand_bool() ? g_variant_new_boolean(FALSE) : NULL);
+        }
+
+check_roundtrip:
+        _writer_new_connection_reread(con2,
+                                      TEST_SCRATCH_DIR,
+                                      &testfile,
+                                      NO_EXPECTED,
+                                      &reread,
+                                      &reread_same);
+        g_assert(NM_IS_CONNECTION(reread));
+        g_assert(reread_same);
+    }
 }
 
 static void