tc: merge branch 'bg/qdisc-sync-rh1815875'

https://bugzilla.redhat.com/show_bug.cgi?id=1815875
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/516

10 files changed, 215 insertions, 12 deletions

diff --git a/.gitignore b/.gitignore
index 78aca2baef..cfb77444e8 100644
--- a/.gitignore
+++ b/.gitignore
@@ -239,6 +239,8 @@ test-*.trs
 /src/platform/tests/test-platform-general
 /src/platform/tests/test-route-fake
 /src/platform/tests/test-route-linux
+/src/platform/tests/test-tc-fake
+/src/platform/tests/test-tc-linux
 /src/settings/plugins/ifcfg-rh/nmdbus-ifcfg-rh.[ch]
 /src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh
 /src/settings/plugins/ifupdown/tests/test-ifupdown
diff --git a/Makefile.am b/Makefile.am
index ae3f1fc006..f1a06dbf33 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -3848,6 +3848,8 @@ check_programs += \
 	src/platform/tests/test-platform-general \
 	src/platform/tests/test-route-fake \
 	src/platform/tests/test-route-linux \
+	src/platform/tests/test-tc-fake \
+	src/platform/tests/test-tc-linux \
 	$(NULL)
 
 src_platform_tests_monitor_CPPFLAGS = $(src_cppflags_test)
@@ -3902,6 +3904,17 @@ src_platform_tests_test_route_linux_CPPFLAGS = $(src_tests_cppflags_linux)
 src_platform_tests_test_route_linux_LDFLAGS = $(src_platform_tests_ldflags)
 src_platform_tests_test_route_linux_LDADD = $(src_platform_tests_libadd)
 
+src_platform_tests_test_tc_fake_SOURCES = src/platform/tests/test-tc.c
+src_platform_tests_test_tc_fake_CPPFLAGS = $(src_tests_cppflags_fake)
+src_platform_tests_test_tc_fake_LDFLAGS = $(src_platform_tests_ldflags)
+src_platform_tests_test_tc_fake_LDADD = $(src_platform_tests_libadd)
+
+src_platform_tests_test_tc_linux_SOURCES = src/platform/tests/test-tc.c
+src_platform_tests_test_tc_linux_CPPFLAGS = $(src_tests_cppflags_linux)
+src_platform_tests_test_tc_linux_LDFLAGS = $(src_platform_tests_ldflags)
+src_platform_tests_test_tc_linux_LDADD = $(src_platform_tests_libadd)
+
+
 $(src_platform_tests_monitor_OBJECTS):               $(libnm_core_lib_h_pub_mkenums)
 $(src_platform_tests_test_address_fake_OBJECTS):     $(libnm_core_lib_h_pub_mkenums)
 $(src_platform_tests_test_address_linux_OBJECTS):    $(libnm_core_lib_h_pub_mkenums)
@@ -3913,6 +3926,8 @@ $(src_platform_tests_test_nmp_object_OBJECTS):       $(libnm_core_lib_h_pub_mken
 $(src_platform_tests_test_platform_general_OBJECTS): $(libnm_core_lib_h_pub_mkenums)
 $(src_platform_tests_test_route_fake_OBJECTS):       $(libnm_core_lib_h_pub_mkenums)
 $(src_platform_tests_test_route_linux_OBJECTS):      $(libnm_core_lib_h_pub_mkenums)
+$(src_platform_tests_test_tc_fake_OBJECTS):          $(libnm_core_lib_h_pub_mkenums)
+$(src_platform_tests_test_tc_linux_OBJECTS):         $(libnm_core_lib_h_pub_mkenums)
 
 EXTRA_DIST += \
 	src/platform/tests/meson.build \
diff --git a/contrib/fedora/REQUIRED_PACKAGES b/contrib/fedora/REQUIRED_PACKAGES
index f03eebfb00..18ba364199 100755
--- a/contrib/fedora/REQUIRED_PACKAGES
+++ b/contrib/fedora/REQUIRED_PACKAGES
@@ -86,6 +86,7 @@ install \
 # some packages don't exist in certain distributions. Install them one-by-one, and ignore errors.
 install_ignore_missing \
     dbus-python \
+    iproute-tc \
     libasan \
     libpsl-devel \
     libubsan \
diff --git a/contrib/fedora/rpm/NetworkManager.spec b/contrib/fedora/rpm/NetworkManager.spec
index 59406e5b23..81567ab58a 100644
--- a/contrib/fedora/rpm/NetworkManager.spec
+++ b/contrib/fedora/rpm/NetworkManager.spec
@@ -250,6 +250,10 @@ BuildRequires: libubsan
 %if %{with firewalld_zone}
 BuildRequires: firewalld-filesystem
 %endif
+BuildRequires: iproute
+%if 0%{?fedora} || 0%{?rhel} > 7
+BuildRequires: iproute-tc
+%endif
 
 Provides: %{name}-dispatcher%{?_isa} = %{epoch}:%{version}-%{release}
 
diff --git a/src/platform/nm-linux-platform.c b/src/platform/nm-linux-platform.c
index a721fc7cfa..78e33748c1 100644
--- a/src/platform/nm-linux-platform.c
+++ b/src/platform/nm-linux-platform.c
@@ -4644,7 +4644,7 @@ _nl_msg_new_qdisc (int nlmsg_type,
 		.tcm_info = qdisc->info,
 	};
 
-	msg = nlmsg_alloc_simple (nlmsg_type, nlmsg_flags);
+	msg = nlmsg_alloc_simple (nlmsg_type, nlmsg_flags | NMP_NLM_FLAG_F_ECHO);
 
 	if (nlmsg_append_struct (msg, &tcm) < 0)
 		goto nla_put_failure;
@@ -4697,7 +4697,7 @@ _nl_msg_new_tfilter (int nlmsg_type,
 		.tcm_info = tfilter->info,
 	};
 
-	msg = nlmsg_alloc_simple (nlmsg_type, nlmsg_flags);
+	msg = nlmsg_alloc_simple (nlmsg_type, nlmsg_flags | NMP_NLM_FLAG_F_ECHO);
 
 	if (nlmsg_append_struct (msg, &tcm) < 0)
 		goto nla_put_failure;
diff --git a/src/platform/nm-platform.c b/src/platform/nm-platform.c
index 8d826a109f..5ac788f73f 100644
--- a/src/platform/nm-platform.c
+++ b/src/platform/nm-platform.c
@@ -310,6 +310,7 @@ NM_UTILS_LOOKUP_STR_DEFINE (_nmp_nlm_flag_to_string_lookup, NMPNlmFlags,
 	NM_UTILS_LOOKUP_ITEM_IGNORE (NMP_NLM_FLAG_F_APPEND),
 	NM_UTILS_LOOKUP_ITEM_IGNORE (NMP_NLM_FLAG_FMASK),
 	NM_UTILS_LOOKUP_ITEM_IGNORE (NMP_NLM_FLAG_SUPPRESS_NETLINK_FAILURE),
+	NM_UTILS_LOOKUP_ITEM_IGNORE (NMP_NLM_FLAG_F_ECHO),
 );
 
 #define _nmp_nlm_flag_to_string(flags) \
@@ -5063,12 +5064,15 @@ nm_platform_qdisc_sync (NMPlatform *self,
 
 	known_qdiscs_idx = g_hash_table_new ((GHashFunc) nmp_object_id_hash,
 	                                     (GEqualFunc) nmp_object_id_equal);
-
 	if (known_qdiscs) {
 		for (i = 0; i < known_qdiscs->len; i++) {
 			const NMPObject *q = g_ptr_array_index (known_qdiscs, i);
 
-			g_hash_table_insert (known_qdiscs_idx, (gpointer) q, (gpointer) q);
+			if (!g_hash_table_insert (known_qdiscs_idx, (gpointer) q, (gpointer) q)) {
+				_LOGW ("duplicate qdisc %s", nm_platform_qdisc_to_string (&q->qdisc, NULL, 0));
+				return FALSE;
+			}
+
 		}
 	}
 
@@ -5077,13 +5081,34 @@ nm_platform_qdisc_sync (NMPlatform *self,
 	                                                                NMP_OBJECT_TYPE_QDISC,
 	                                                                ifindex),
 	                                        NULL, NULL);
-
 	if (plat_qdiscs) {
 		for (i = 0; i < plat_qdiscs->len; i++) {
-			const NMPObject *q = g_ptr_array_index (plat_qdiscs, i);
+			const NMPObject *p = g_ptr_array_index (plat_qdiscs, i);
+			const NMPObject *k;
 
-			if (!g_hash_table_lookup (known_qdiscs_idx, q))
-				success &= nm_platform_object_delete (self, q);
+			/* look up known qdisc with same parent */
+			k = g_hash_table_lookup (known_qdiscs_idx, p);
+
+			if (k) {
+				const NMPlatformQdisc *qdisc_k = NMP_OBJECT_CAST_QDISC (k);
+				const NMPlatformQdisc *qdisc_p = NMP_OBJECT_CAST_QDISC (p);
+
+				/* check other fields */
+				if (   nm_platform_qdisc_cmp_full (qdisc_k, qdisc_p, FALSE) != 0
+				    || (   qdisc_k->handle != qdisc_p->handle
+				        && qdisc_k != 0)) {
+					k = NULL;
+				}
+			}
+
+			if (k) {
+				g_hash_table_remove (known_qdiscs_idx, k);
+			} else {
+				/* can't delete qdisc with zero handle */
+				if (TC_H_MAJ (p->qdisc.handle) != 0) {
+					success &= nm_platform_object_delete (self, p);
+				}
+			}
 		}
 	}
 
@@ -5091,8 +5116,10 @@ nm_platform_qdisc_sync (NMPlatform *self,
 		for (i = 0; i < known_qdiscs->len; i++) {
 			const NMPObject *q = g_ptr_array_index (known_qdiscs, i);
 
-			success &= (nm_platform_qdisc_add (self, NMP_NLM_FLAG_ADD,
-			                                   NMP_OBJECT_CAST_QDISC (q)) >= 0);
+			if (g_hash_table_contains (known_qdiscs_idx, q)) {
+				success &= (nm_platform_qdisc_add (self, NMP_NLM_FLAG_ADD,
+				                                   NMP_OBJECT_CAST_QDISC (q)) >= 0);
+			}
 		}
 	}
 
@@ -6517,14 +6544,17 @@ nm_platform_qdisc_hash_update (const NMPlatformQdisc *obj, NMHashState *h)
 }
 
 int
-nm_platform_qdisc_cmp (const NMPlatformQdisc *a, const NMPlatformQdisc *b)
+nm_platform_qdisc_cmp_full (const NMPlatformQdisc *a,
+                            const NMPlatformQdisc *b,
+                            gboolean compare_handle)
 {
 	NM_CMP_SELF (a, b);
 	NM_CMP_FIELD (a, b, ifindex);
 	NM_CMP_FIELD (a, b, parent);
 	NM_CMP_FIELD_STR_INTERNED (a, b, kind);
 	NM_CMP_FIELD (a, b, addr_family);
-	NM_CMP_FIELD (a, b, handle);
+	if (compare_handle)
+		NM_CMP_FIELD (a, b, handle);
 	NM_CMP_FIELD (a, b, info);
 
 	if (nm_streq0 (a->kind, "fq_codel")) {
@@ -6541,6 +6571,12 @@ nm_platform_qdisc_cmp (const NMPlatformQdisc *a, const NMPlatformQdisc *b)
 	return 0;
 }
 
+int
+nm_platform_qdisc_cmp (const NMPlatformQdisc *a, const NMPlatformQdisc *b)
+{
+	return nm_platform_qdisc_cmp_full (a, b, TRUE);
+}
+
 const char *
 nm_platform_tfilter_to_string (const NMPlatformTfilter *tfilter, char *buf, gsize len)
 {
diff --git a/src/platform/nm-platform.h b/src/platform/nm-platform.h
index c21190e803..fb82262979 100644
--- a/src/platform/nm-platform.h
+++ b/src/platform/nm-platform.h
@@ -69,6 +69,8 @@ typedef gboolean (*NMPObjectPredicateFunc) (const NMPObject *obj,
 #define NM_GRE_KEY      0x2000
 
 typedef enum {
+	NMP_NLM_FLAG_F_ECHO         = 0x08, /* NLM_F_ECHO, Echo this request */
+
 	/* use our own platform enum for the nlmsg-flags. Otherwise, we'd have
 	 * to include <linux/netlink.h> */
 	NMP_NLM_FLAG_F_REPLACE      = 0x100, /* NLM_F_REPLACE, Override existing */
@@ -1907,6 +1909,9 @@ nm_platform_routing_rule_cmp_full (const NMPlatformRoutingRule *a, const NMPlatf
 }
 
 int nm_platform_qdisc_cmp (const NMPlatformQdisc *a, const NMPlatformQdisc *b);
+int nm_platform_qdisc_cmp_full (const NMPlatformQdisc *a,
+                                const NMPlatformQdisc *b,
+                                gboolean compare_handle);
 int nm_platform_tfilter_cmp (const NMPlatformTfilter *a, const NMPlatformTfilter *b);
 
 void nm_platform_link_hash_update (const NMPlatformLink *obj, NMHashState *h);
diff --git a/src/platform/tests/meson.build b/src/platform/tests/meson.build
index 4a50bca978..f96850cc26 100644
--- a/src/platform/tests/meson.build
+++ b/src/platform/tests/meson.build
@@ -14,6 +14,8 @@ test_units = [
   ['test-platform-general', 'test-platform-general.c', test_c_flags, default_test_timeout],
   ['test-route-fake', 'test-route.c', test_fake_c_flags, default_test_timeout],
   ['test-route-linux', 'test-route.c', test_linux_c_flags, default_test_timeout],
+  ['test-tc-fake', 'test-tc.c', test_fake_c_flags, default_test_timeout],
+  ['test-tc-linux', 'test-tc.c', test_linux_c_flags, default_test_timeout],
 ]
 
 foreach test_unit: test_units
diff --git a/src/platform/tests/test-common.c b/src/platform/tests/test-common.c
index f28dfa3a72..9f6a29bc99 100644
--- a/src/platform/tests/test-common.c
+++ b/src/platform/tests/test-common.c
@@ -557,6 +557,7 @@ nmtstp_wait_for_signal (NMPlatform *platform, gint64 timeout_msec)
 {
 	WaitForSignalData data = { 0 };
 	gulong id_link, id_ip4_address, id_ip6_address, id_ip4_route, id_ip6_route;
+	gulong id_qdisc, id_tfilter;
 
 	_init_platform (&platform, FALSE);
 
@@ -567,6 +568,8 @@ nmtstp_wait_for_signal (NMPlatform *platform, gint64 timeout_msec)
 	id_ip6_address = g_signal_connect (platform, NM_PLATFORM_SIGNAL_IP6_ADDRESS_CHANGED, G_CALLBACK (_wait_for_signal_cb), &data);
 	id_ip4_route   = g_signal_connect (platform, NM_PLATFORM_SIGNAL_IP4_ROUTE_CHANGED, G_CALLBACK (_wait_for_signal_cb), &data);
 	id_ip6_route   = g_signal_connect (platform, NM_PLATFORM_SIGNAL_IP6_ROUTE_CHANGED, G_CALLBACK (_wait_for_signal_cb), &data);
+	id_qdisc       = g_signal_connect (platform, NM_PLATFORM_SIGNAL_QDISC_CHANGED, G_CALLBACK (_wait_for_signal_cb), &data);
+	id_tfilter     = g_signal_connect (platform, NM_PLATFORM_SIGNAL_TFILTER_CHANGED, G_CALLBACK (_wait_for_signal_cb), &data);
 
 	/* if timeout_msec is negative, it means the wait-time already expired.
 	 * Maybe, we should do nothing and return right away, without even
@@ -589,6 +592,8 @@ nmtstp_wait_for_signal (NMPlatform *platform, gint64 timeout_msec)
 	g_assert (nm_clear_g_signal_handler (platform, &id_ip6_address));
 	g_assert (nm_clear_g_signal_handler (platform, &id_ip4_route));
 	g_assert (nm_clear_g_signal_handler (platform, &id_ip6_route));
+	g_assert (nm_clear_g_signal_handler (platform, &id_tfilter));
+	g_assert (nm_clear_g_signal_handler (platform, &id_qdisc));
 
 	nm_clear_pointer (&data.loop, g_main_loop_unref);
 
diff --git a/src/platform/tests/test-tc.c b/src/platform/tests/test-tc.c
new file mode 100644
index 0000000000..dc2bf9f214
--- /dev/null
+++ b/src/platform/tests/test-tc.c
@@ -0,0 +1,133 @@
+// SPDX-License-Identifier: LGPL-2.1+
+
+#include "nm-default.h"
+
+#include <linux/pkt_sched.h>
+
+#include "nm-test-utils-core.h"
+#include "platform/nmp-object.h"
+#include "platform/nmp-netns.h"
+#include "platform/nm-platform-utils.h"
+#include "test-common.h"
+
+static NMPObject *
+qdisc_new (int ifindex, const char *kind, guint32 parent)
+{
+	NMPObject *obj;
+
+	obj = nmp_object_new (NMP_OBJECT_TYPE_QDISC, NULL);
+	obj->qdisc = (NMPlatformQdisc) {
+		.ifindex = ifindex,
+		.kind = kind,
+		.parent = parent,
+	};
+
+	return obj;
+}
+
+static GPtrArray *
+qdiscs_lookup (int ifindex)
+{
+	NMPLookup lookup;
+
+	return nm_platform_lookup_clone (NM_PLATFORM_GET,
+	                                 nmp_lookup_init_object (&lookup,
+	                                                         NMP_OBJECT_TYPE_QDISC,
+	                                                         ifindex),
+	                                 NULL, NULL);
+}
+
+static void
+test_qdisc1 (void)
+{
+	int ifindex;
+	gs_unref_ptrarray GPtrArray *known = NULL;
+	gs_unref_ptrarray GPtrArray *plat = NULL;
+	NMPObject *obj;
+	NMPlatformQdisc *qdisc;
+
+	ifindex = nm_platform_link_get_ifindex (NM_PLATFORM_GET, DEVICE_NAME);
+	g_assert_cmpint (ifindex, >, 0);
+
+	nmtstp_run_command       ("tc qdisc del dev %s root", DEVICE_NAME);
+	nmtstp_run_command_check ("tc qdisc add dev %s root sfq", DEVICE_NAME);
+
+	nmtstp_wait_for_signal (NM_PLATFORM_GET, 0);
+
+	known = g_ptr_array_new_with_free_func ((GDestroyNotify) nmp_object_unref);
+	g_ptr_array_add (known, qdisc_new (ifindex, "fq_codel", TC_H_ROOT));
+	g_ptr_array_add (known, qdisc_new (ifindex, "ingress", TC_H_INGRESS));
+
+	g_assert (nm_platform_qdisc_sync (NM_PLATFORM_GET, ifindex, known));
+	plat = qdiscs_lookup (ifindex);
+	g_assert (plat);
+	g_assert_cmpint (plat->len, ==, 2);
+
+	obj = plat->pdata[0];
+	qdisc = NMP_OBJECT_CAST_QDISC (obj);
+	g_assert_cmpint (qdisc->parent, ==, TC_H_ROOT);
+	g_assert_cmpstr (qdisc->kind, ==, "fq_codel");
+
+	obj = plat->pdata[1];
+	qdisc = NMP_OBJECT_CAST_QDISC (obj);
+	g_assert_cmpint (qdisc->parent, ==, TC_H_INGRESS);
+	g_assert_cmpstr (qdisc->kind, ==, "ingress");
+}
+
+static void
+test_qdisc2 (void)
+{
+	int ifindex;
+	gs_unref_ptrarray GPtrArray *known = NULL;
+	gs_unref_ptrarray GPtrArray *plat = NULL;
+	NMPObject *obj;
+	NMPlatformQdisc *qdisc;
+
+	ifindex = nm_platform_link_get_ifindex (NM_PLATFORM_GET, DEVICE_NAME);
+	g_assert_cmpint (ifindex, >, 0);
+
+	nmtstp_run_command ("tc qdisc del dev %s root", DEVICE_NAME);
+
+	nmtstp_wait_for_signal (NM_PLATFORM_GET, 0);
+
+	known = g_ptr_array_new_with_free_func ((GDestroyNotify) nmp_object_unref);
+	obj = qdisc_new (ifindex, "fq_codel", TC_H_ROOT);
+	obj->qdisc.handle = TC_H_MAKE (0x8142 << 16, 0);
+	obj->qdisc.fq_codel.limit = 2048;
+	obj->qdisc.fq_codel.flows = 64;
+	obj->qdisc.fq_codel.quantum = 1000;
+	g_ptr_array_add (known, obj);
+
+	g_assert (nm_platform_qdisc_sync (NM_PLATFORM_GET, ifindex, known));
+	plat = qdiscs_lookup (ifindex);
+	g_assert (plat);
+	g_assert_cmpint (plat->len, ==, 1);
+
+	obj = plat->pdata[0];
+	qdisc = NMP_OBJECT_CAST_QDISC (obj);
+	g_assert_cmpstr (qdisc->kind, ==, "fq_codel");
+	g_assert_cmpint (qdisc->handle, ==, TC_H_MAKE (0x8142 << 16, 0));
+	g_assert_cmpint (qdisc->parent, ==, TC_H_ROOT);
+	g_assert_cmpint (qdisc->fq_codel.limit, ==, 2048);
+	g_assert_cmpint (qdisc->fq_codel.flows, ==, 64);
+	g_assert_cmpint (qdisc->fq_codel.quantum, ==, 1000);
+}
+
+/*****************************************************************************/
+
+NMTstpSetupFunc const _nmtstp_setup_platform_func = SETUP;
+
+void
+_nmtstp_init_tests (int *argc, char ***argv)
+{
+	nmtst_init_with_logging (argc, argv, NULL, "ALL");
+}
+
+void
+_nmtstp_setup_tests (void)
+{
+	if (nmtstp_is_root_test ()) {
+		nmtstp_env1_add_test_func ("/link/qdisc/1", test_qdisc1, TRUE);
+		nmtstp_env1_add_test_func ("/link/qdisc/2", test_qdisc2, TRUE);
+	}
+}