From 67047a444d45dd6e74d381cfe06a418c13693539 Mon Sep 17 00:00:00 2001
From: Lubomir Rintel <lkundrak@v3.sk>
Date: Tue, 9 Apr 2019 13:42:52 +0200
Subject: [PATCH 01/20] tc/qdisc: add support for fq_codel attributes

(cherry picked from commit 1efe982e39be7e8b7852a19957c7b49cab46e67c)
---
 libnm-core/nm-utils.c            | 13 ++++++
 src/devices/nm-device.c          | 23 ++++++++++
 src/platform/nm-linux-platform.c | 72 ++++++++++++++++++++++++++++++++
 src/platform/nm-platform.c       | 55 ++++++++++++++++++++----
 src/platform/nm-platform.h       | 14 +++++++
 5 files changed, 170 insertions(+), 7 deletions(-)

diff --git a/libnm-core/nm-utils.c b/libnm-core/nm-utils.c
index 04d5b1b558..b82c580345 100644
--- a/libnm-core/nm-utils.c
+++ b/libnm-core/nm-utils.c
@@ -2309,12 +2309,25 @@ static const NMVariantAttributeSpec * const tc_object_attribute_spec[] = {
	NULL,
 };

+static const NMVariantAttributeSpec * const tc_qdisc_fq_codel_spec[] = {
+	TC_ATTR_SPEC_PTR ("limit",        G_VARIANT_TYPE_UINT32,  FALSE, FALSE, 0   ),
+	TC_ATTR_SPEC_PTR ("flows",        G_VARIANT_TYPE_UINT32,  FALSE, FALSE, 0   ),
+	TC_ATTR_SPEC_PTR ("target",       G_VARIANT_TYPE_UINT32,  FALSE, FALSE, 0   ),
+	TC_ATTR_SPEC_PTR ("interval",     G_VARIANT_TYPE_UINT32,  FALSE, FALSE, 0   ),
+	TC_ATTR_SPEC_PTR ("quantum",      G_VARIANT_TYPE_UINT32,  FALSE, FALSE, 0   ),
+	TC_ATTR_SPEC_PTR ("ce_threshold", G_VARIANT_TYPE_UINT32,  FALSE, FALSE, 0   ),
+	TC_ATTR_SPEC_PTR ("memory",       G_VARIANT_TYPE_UINT32,  FALSE, FALSE, 0   ),
+	TC_ATTR_SPEC_PTR ("ecn",          G_VARIANT_TYPE_BOOLEAN, TRUE,  FALSE, 0   ),
+	NULL,
+};
+
 typedef struct {
	const char *kind;
	const NMVariantAttributeSpec * const *attrs;
 } NMQdiscAttributeSpec;

 static const NMQdiscAttributeSpec *const tc_qdisc_attribute_spec[] = {
+	&(const NMQdiscAttributeSpec) { "fq_codel", tc_qdisc_fq_codel_spec },
	NULL,
 };

diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c
index 60709c50d5..195686707b 100644
--- a/src/devices/nm-device.c
+++ b/src/devices/nm-device.c
@@ -6522,6 +6522,29 @@ tc_commit (NMDevice *self)
			qdisc->parent = nm_tc_qdisc_get_parent (s_qdisc);
			qdisc->info = 0;

+#define GET_ATTR(name, dst, variant_type, type, dflt) G_STMT_START { \
+	GVariant *_variant = nm_tc_qdisc_get_attribute (s_qdisc, ""name""); \
+	\
+	if (   _variant \
+	    && g_variant_is_of_type (_variant, G_VARIANT_TYPE_ ## variant_type)) \
+		(dst) = g_variant_get_ ## type (_variant); \
+	else \
+		(dst) = (dflt); \
+} G_STMT_END
+
+			if (strcmp (qdisc->kind, "fq_codel") == 0) {
+				GET_ATTR("limit", qdisc->fq_codel.limit, UINT32, uint32, 0);
+				GET_ATTR("flows", qdisc->fq_codel.flows, UINT32, uint32, 0);
+				GET_ATTR("target", qdisc->fq_codel.target, UINT32, uint32, 0);
+				GET_ATTR("interval", qdisc->fq_codel.interval, UINT32, uint32, 0);
+				GET_ATTR("quantum", qdisc->fq_codel.quantum, UINT32, uint32, 0);
+				GET_ATTR("ce_threshold", qdisc->fq_codel.ce_threshold, UINT32, uint32, -1);
+				GET_ATTR("memory", qdisc->fq_codel.memory, UINT32, uint32, -1);
+				GET_ATTR("ecn", qdisc->fq_codel.ecn, BOOLEAN, boolean, FALSE);
+			}
+
+#undef GET_ADDR
+
			g_ptr_array_add (qdiscs, q);
		}

diff --git a/src/platform/nm-linux-platform.c b/src/platform/nm-linux-platform.c
index d4b0115252..a346d6618c 100644
--- a/src/platform/nm-linux-platform.c
+++ b/src/platform/nm-linux-platform.c
@@ -84,6 +84,13 @@ enum {

 /*****************************************************************************/

+/* Compat with older kernels. */
+
+#define TCA_FQ_CODEL_CE_THRESHOLD 7
+#define TCA_FQ_CODEL_MEMORY_LIMIT 9
+
+/*****************************************************************************/
+
 #define VLAN_FLAG_MVRP 0x8

 /*****************************************************************************/
@@ -3481,6 +3488,7 @@ _new_from_nl_qdisc (struct nlmsghdr *nlh, gboolean id_only)
 {
	static const struct nla_policy policy[] = {
		[TCA_KIND] = { .type = NLA_STRING },
+		[TCA_OPTIONS] = { .type = NLA_NESTED },
	};
	struct nlattr *tb[G_N_ELEMENTS (policy)];
	const struct tcmsg *tcm;
@@ -3506,6 +3514,45 @@ _new_from_nl_qdisc (struct nlmsghdr *nlh, gboolean id_only)
	obj->qdisc.parent = tcm->tcm_parent;
	obj->qdisc.info = tcm->tcm_info;

+	if (tb[TCA_OPTIONS]) {
+		struct nlattr *options_attr;
+		int remaining;
+
+		nla_for_each_nested (options_attr, tb[TCA_OPTIONS], remaining) {
+			if (nla_len (options_attr) < sizeof (uint32_t))
+				continue;
+
+			if (nm_streq0 (obj->qdisc.kind, "fq_codel")) {
+				switch (nla_type (options_attr)) {
+				case TCA_FQ_CODEL_LIMIT:
+					obj->qdisc.fq_codel.limit = nla_get_u32 (options_attr);
+					break;
+				case TCA_FQ_CODEL_FLOWS:
+					obj->qdisc.fq_codel.flows = nla_get_u32 (options_attr);
+					break;
+				case TCA_FQ_CODEL_TARGET:
+					obj->qdisc.fq_codel.target = nla_get_u32 (options_attr);
+					break;
+				case TCA_FQ_CODEL_INTERVAL:
+					obj->qdisc.fq_codel.interval = nla_get_u32 (options_attr);
+					break;
+				case TCA_FQ_CODEL_QUANTUM:
+					obj->qdisc.fq_codel.quantum = nla_get_u32 (options_attr);
+					break;
+				case TCA_FQ_CODEL_CE_THRESHOLD:
+					obj->qdisc.fq_codel.ce_threshold = nla_get_u32 (options_attr);
+					break;
+				case TCA_FQ_CODEL_MEMORY_LIMIT:
+					obj->qdisc.fq_codel.memory = nla_get_u32 (options_attr);
+					break;
+				case TCA_FQ_CODEL_ECN:
+					obj->qdisc.fq_codel.ecn = nla_get_u32 (options_attr);
+					break;
+				}
+			}
+		}
+	}
+
	return obj;
 }

@@ -4161,6 +4208,7 @@ _nl_msg_new_qdisc (int nlmsg_type,
                    const NMPlatformQdisc *qdisc)
 {
	nm_auto_nlmsg struct nl_msg *msg = NULL;
+	struct nlattr *tc_options;
	const struct tcmsg tcm = {
		.tcm_family = qdisc->addr_family,
		.tcm_ifindex = qdisc->ifindex,
@@ -4176,6 +4224,30 @@ _nl_msg_new_qdisc (int nlmsg_type,

	NLA_PUT_STRING (msg, TCA_KIND, qdisc->kind);

+	if (!(tc_options = nla_nest_start (msg, TCA_OPTIONS)))
+		goto nla_put_failure;
+
+	if (strcmp (qdisc->kind, "fq_codel") == 0) {
+		if (qdisc->fq_codel.limit)
+			NLA_PUT_U32 (msg, TCA_FQ_CODEL_LIMIT, qdisc->fq_codel.limit);
+		if (qdisc->fq_codel.flows)
+			NLA_PUT_U32 (msg, TCA_FQ_CODEL_FLOWS, qdisc->fq_codel.flows);
+		if (qdisc->fq_codel.target)
+			NLA_PUT_U32 (msg, TCA_FQ_CODEL_TARGET, qdisc->fq_codel.target);
+		if (qdisc->fq_codel.interval)
+			NLA_PUT_U32 (msg, TCA_FQ_CODEL_INTERVAL, qdisc->fq_codel.interval);
+		if (qdisc->fq_codel.quantum)
+			NLA_PUT_U32 (msg, TCA_FQ_CODEL_QUANTUM, qdisc->fq_codel.quantum);
+		if (qdisc->fq_codel.ce_threshold != -1)
+			NLA_PUT_U32 (msg, TCA_FQ_CODEL_CE_THRESHOLD, qdisc->fq_codel.ce_threshold);
+		if (qdisc->fq_codel.memory != -1)
+			NLA_PUT_U32 (msg, TCA_FQ_CODEL_MEMORY_LIMIT, qdisc->fq_codel.memory);
+		if (qdisc->fq_codel.ecn)
+			NLA_PUT_S32 (msg, TCA_FQ_CODEL_ECN, qdisc->fq_codel.ecn);
+	}
+
+	nla_nest_end (msg, tc_options);
+
	return g_steal_pointer (&msg);

 nla_put_failure:
diff --git a/src/platform/nm-platform.c b/src/platform/nm-platform.c
index 1fc0ccb750..3d78902860 100644
--- a/src/platform/nm-platform.c
+++ b/src/platform/nm-platform.c
@@ -6430,13 +6430,32 @@ nm_platform_qdisc_to_string (const NMPlatformQdisc *qdisc, char *buf, gsize len)
	if (!nm_utils_to_string_buffer_init_null (qdisc, &buf, &len))
		return buf;

-	g_snprintf (buf, len, "%s%s family %d handle %x parent %x info %x",
-	            qdisc->kind,
-	            _to_string_dev (NULL, qdisc->ifindex, str_dev, sizeof (str_dev)),
-	            qdisc->addr_family,
-	            qdisc->handle,
-	            qdisc->parent,
-	            qdisc->info);
+	nm_utils_strbuf_append (&buf, &len, "%s%s family %u handle %x parent %x info %x",
+	                        qdisc->kind,
+	                        _to_string_dev (NULL, qdisc->ifindex, str_dev, sizeof (str_dev)),
+	                        qdisc->addr_family,
+	                        qdisc->handle,
+	                        qdisc->parent,
+	                        qdisc->info);
+
+	if (nm_streq0 (qdisc->kind, "fq_codel")) {
+		if (qdisc->fq_codel.limit)
+			nm_utils_strbuf_append (&buf, &len, " limit %u", qdisc->fq_codel.limit);
+		if (qdisc->fq_codel.flows)
+			nm_utils_strbuf_append (&buf, &len, " flows %u", qdisc->fq_codel.flows);
+		if (qdisc->fq_codel.target)
+			nm_utils_strbuf_append (&buf, &len, " target %u", qdisc->fq_codel.target);
+		if (qdisc->fq_codel.interval)
+			nm_utils_strbuf_append (&buf, &len, " interval %u", qdisc->fq_codel.interval);
+		if (qdisc->fq_codel.quantum)
+			nm_utils_strbuf_append (&buf, &len, " quantum %u", qdisc->fq_codel.quantum);
+		if (qdisc->fq_codel.ce_threshold != -1)
+			nm_utils_strbuf_append (&buf, &len, " ce_threshold %u", qdisc->fq_codel.ce_threshold);
+		if (qdisc->fq_codel.memory != -1)
+			nm_utils_strbuf_append (&buf, &len, " memory %u", qdisc->fq_codel.memory);
+		if (qdisc->fq_codel.ecn)
+			nm_utils_strbuf_append (&buf, &len, " ecn");
+	}

	return buf;
 }
@@ -6451,6 +6470,17 @@ nm_platform_qdisc_hash_update (const NMPlatformQdisc *obj, NMHashState *h)
	                     obj->handle,
	                     obj->parent,
	                     obj->info);
+	if (nm_streq0 (obj->kind, "fq_codel")) {
+		nm_hash_update_vals (h,
+		                     obj->fq_codel.limit,
+		                     obj->fq_codel.flows,
+		                     obj->fq_codel.target,
+		                     obj->fq_codel.interval,
+		                     obj->fq_codel.quantum,
+		                     obj->fq_codel.ce_threshold,
+		                     obj->fq_codel.memory,
+		                     obj->fq_codel.ecn == TRUE);
+	}
 }

 int
@@ -6464,6 +6494,17 @@ nm_platform_qdisc_cmp (const NMPlatformQdisc *a, const NMPlatformQdisc *b)
	NM_CMP_FIELD (a, b, handle);
	NM_CMP_FIELD (a, b, info);

+	if (nm_streq0 (a->kind, "fq_codel")) {
+		NM_CMP_FIELD (a, b, fq_codel.limit);
+		NM_CMP_FIELD (a, b, fq_codel.flows);
+		NM_CMP_FIELD (a, b, fq_codel.target);
+		NM_CMP_FIELD (a, b, fq_codel.interval);
+		NM_CMP_FIELD (a, b, fq_codel.quantum);
+		NM_CMP_FIELD (a, b, fq_codel.ce_threshold);
+		NM_CMP_FIELD (a, b, fq_codel.memory);
+		NM_CMP_FIELD (a, b, fq_codel.ecn == TRUE);
+	}
+
	return 0;
 }

diff --git a/src/platform/nm-platform.h b/src/platform/nm-platform.h
index 97be88324e..8742b90555 100644
--- a/src/platform/nm-platform.h
+++ b/src/platform/nm-platform.h
@@ -596,6 +596,17 @@ typedef struct {
	bool     uid_range_has:1;            /* has(FRA_UID_RANGE) */
 } NMPlatformRoutingRule;

+typedef struct {
+	guint32 limit;
+	guint32 flows;
+	guint32 target;
+	guint32 interval;
+	guint32 quantum;
+	guint32 ce_threshold;
+	guint32 memory;
+	bool ecn:1;
+} NMPlatformQdiscFqCodel;
+
 typedef struct {
	__NMPlatformObjWithIfindex_COMMON;
	const char *kind;
@@ -603,6 +614,9 @@ typedef struct {
	guint32 handle;
	guint32 parent;
	guint32 info;
+	union {
+		NMPlatformQdiscFqCodel fq_codel;
+	};
 } NMPlatformQdisc;

 typedef struct {
--
2.21.0

From 4be7cf71e06da4801d8bb066b977c733e7e7097c Mon Sep 17 00:00:00 2001
From: Lubomir Rintel <lkundrak@v3.sk>
Date: Tue, 9 Apr 2019 16:23:39 +0200
Subject: [PATCH 02/20] tc/tfilter: add mirred action

(cherry picked from commit 900292147d8fd584479a7af0881984c2d77a60bf)
---
 libnm-core/nm-utils.c            | 11 +++++++++++
 src/devices/nm-device.c          | 29 +++++++++++++++++++++++-----
 src/platform/nm-linux-platform.c | 33 ++++++++++++++++++++++++++++++++
 src/platform/nm-platform.c       | 29 +++++++++++++++++++++++++---
 src/platform/nm-platform.h       | 10 ++++++++++
 5 files changed, 104 insertions(+), 8 deletions(-)

diff --git a/libnm-core/nm-utils.c b/libnm-core/nm-utils.c
index b82c580345..3af3e04ed9 100644
--- a/libnm-core/nm-utils.c
+++ b/libnm-core/nm-utils.c
@@ -2542,6 +2542,15 @@ static const NMVariantAttributeSpec * const tc_action_simple_attribute_spec[] =
	NULL,
 };

+static const NMVariantAttributeSpec * const tc_action_mirred_attribute_spec[] = {
+	TC_ATTR_SPEC_PTR ("egress",   G_VARIANT_TYPE_BOOLEAN, TRUE,  FALSE, 0   ),
+	TC_ATTR_SPEC_PTR ("ingress",  G_VARIANT_TYPE_BOOLEAN, TRUE,  FALSE, 0   ),
+	TC_ATTR_SPEC_PTR ("mirror",   G_VARIANT_TYPE_BOOLEAN, TRUE,  FALSE, 0   ),
+	TC_ATTR_SPEC_PTR ("redirect", G_VARIANT_TYPE_BOOLEAN, TRUE,  FALSE, 0   ),
+	TC_ATTR_SPEC_PTR ("dev",      G_VARIANT_TYPE_STRING,  TRUE,  FALSE, 'a' ),
+	NULL,
+};
+
 static const NMVariantAttributeSpec * const tc_action_attribute_spec[] = {
	TC_ATTR_SPEC_PTR ("kind",    G_VARIANT_TYPE_STRING,      TRUE,  FALSE, 'a' ),
	TC_ATTR_SPEC_PTR ("",        G_VARIANT_TYPE_STRING,      TRUE,  TRUE,  'a' ),
@@ -2636,6 +2645,8 @@ nm_utils_tc_action_from_str (const char *str, GError **error)
	kind = g_variant_get_string (variant, NULL);
	if (strcmp (kind, "simple") == 0)
		attrs = tc_action_simple_attribute_spec;
+	else if (strcmp (kind, "mirred") == 0)
+		attrs = tc_action_mirred_attribute_spec;
	else
		attrs = NULL;

diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c
index 195686707b..ea6ad7e2ad 100644
--- a/src/devices/nm-device.c
+++ b/src/devices/nm-device.c
@@ -6566,16 +6566,35 @@ tc_commit (NMDevice *self)

			action = nm_tc_tfilter_get_action (s_tfilter);
			if (action) {
+				GVariant *var;
+
				tfilter->action.kind = nm_tc_action_get_kind (action);
				if (strcmp (tfilter->action.kind, "simple") == 0) {
-					GVariant *sdata;
-
-					sdata = nm_tc_action_get_attribute (action, "sdata");
-					if (sdata && g_variant_is_of_type (sdata, G_VARIANT_TYPE_BYTESTRING)) {
+					var = nm_tc_action_get_attribute (action, "sdata");
+					if (var && g_variant_is_of_type (var, G_VARIANT_TYPE_BYTESTRING)) {
						g_strlcpy (tfilter->action.simple.sdata,
-						           g_variant_get_bytestring (sdata),
+						           g_variant_get_bytestring (var),
						           sizeof (tfilter->action.simple.sdata));
					}
+				} else if (strcmp (tfilter->action.kind, "mirred") == 0) {
+					if (nm_tc_action_get_attribute (action, "egress"))
+						tfilter->action.mirred.egress = TRUE;
+
+					if (nm_tc_action_get_attribute (action, "ingress"))
+						tfilter->action.mirred.ingress = TRUE;
+
+					if (nm_tc_action_get_attribute (action, "mirror"))
+						tfilter->action.mirred.mirror = TRUE;
+
+					if (nm_tc_action_get_attribute (action, "redirect"))
+						tfilter->action.mirred.redirect = TRUE;
+
+					var = nm_tc_action_get_attribute (action, "dev");
+					if (var && g_variant_is_of_type (var, G_VARIANT_TYPE_STRING)) {
+						int ifindex = nm_platform_link_get_ifindex (nm_device_get_platform (self),
+						                                            g_variant_get_string (var, NULL));
+						tfilter->action.mirred.ifindex = ifindex;
+					}
				}
			}

diff --git a/src/platform/nm-linux-platform.c b/src/platform/nm-linux-platform.c
index a346d6618c..6064d89eb6 100644
--- a/src/platform/nm-linux-platform.c
+++ b/src/platform/nm-linux-platform.c
@@ -34,6 +34,7 @@
 #include <linux/if_tun.h>
 #include <linux/if_tunnel.h>
 #include <linux/ip6_tunnel.h>
+#include <linux/tc_act/tc_mirred.h>
 #include <netinet/icmp6.h>
 #include <netinet/in.h>
 #include <poll.h>
@@ -4275,6 +4276,36 @@ nla_put_failure:
	return FALSE;
 }

+static gboolean
+_add_action_mirred (struct nl_msg *msg,
+                    const NMPlatformActionMirred *mirred)
+{
+	struct nlattr *act_options;
+	struct tc_mirred sel = { 0, };
+
+	if (!(act_options = nla_nest_start (msg, TCA_ACT_OPTIONS)))
+		goto nla_put_failure;
+
+	if (mirred->egress && mirred->redirect)
+		sel.eaction = TCA_EGRESS_REDIR;
+	else if (mirred->egress && mirred->mirror)
+		sel.eaction = TCA_EGRESS_MIRROR;
+	else if (mirred->ingress && mirred->redirect)
+		sel.eaction = TCA_INGRESS_REDIR;
+	else if (mirred->ingress && mirred->mirror)
+		sel.eaction = TCA_INGRESS_MIRROR;
+	sel.ifindex = mirred->ifindex;
+
+	NLA_PUT (msg, TCA_MIRRED_PARMS, sizeof (sel), &sel);
+
+	nla_nest_end (msg, act_options);
+
+	return TRUE;
+
+nla_put_failure:
+	return FALSE;
+}
+
 static gboolean
 _add_action (struct nl_msg *msg,
              const NMPlatformAction *action)
@@ -4290,6 +4321,8 @@ _add_action (struct nl_msg *msg,

	if (nm_streq (action->kind, NM_PLATFORM_ACTION_KIND_SIMPLE))
		_add_action_simple (msg, &action->simple);
+	else if (nm_streq (action->kind, NM_PLATFORM_ACTION_KIND_MIRRED))
+		_add_action_mirred (msg, &action->mirred);

	nla_nest_end (msg, prio);

diff --git a/src/platform/nm-platform.c b/src/platform/nm-platform.c
index 3d78902860..6f23ddb589 100644
--- a/src/platform/nm-platform.c
+++ b/src/platform/nm-platform.c
@@ -34,6 +34,7 @@
 #include <linux/if_tun.h>
 #include <linux/if_tunnel.h>
 #include <linux/rtnetlink.h>
+#include <linux/tc_act/tc_mirred.h>
 #include <libudev.h>

 #include "nm-utils.h"
@@ -6533,11 +6534,18 @@ nm_platform_tfilter_to_string (const NMPlatformTfilter *tfilter, char *buf, gsiz
			                                                        NM_UTILS_STR_UTF8_SAFE_FLAG_ESCAPE_CTRL
			                                                      | NM_UTILS_STR_UTF8_SAFE_FLAG_ESCAPE_NON_ASCII,
			                                                      &t));
+		} else if (nm_streq (tfilter->action.kind, NM_PLATFORM_ACTION_KIND_MIRRED)) {
+			nm_utils_strbuf_append (&p, &l, "%s%s%s%s dev %d",
+			                        tfilter->action.mirred.ingress ? " ingress" : "",
+			                        tfilter->action.mirred.egress ? " egress" : "",
+			                        tfilter->action.mirred.mirror ? " mirror" : "",
+			                        tfilter->action.mirred.redirect ? " redirect" : "",
+			                        tfilter->action.mirred.ifindex);
		}
	} else
		act_buf[0] = '\0';

-	g_snprintf (buf, len, "%s%s family %d handle %x parent %x info %x%s",
+	g_snprintf (buf, len, "%s%s family %u handle %x parent %x info %x%s",
	            tfilter->kind,
	            _to_string_dev (NULL, tfilter->ifindex, str_dev, sizeof (str_dev)),
	            tfilter->addr_family,
@@ -6561,8 +6569,16 @@ nm_platform_tfilter_hash_update (const NMPlatformTfilter *obj, NMHashState *h)
	                     obj->info);
	if (obj->action.kind) {
		nm_hash_update_str (h, obj->action.kind);
-		if (nm_streq (obj->action.kind, NM_PLATFORM_ACTION_KIND_SIMPLE))
+		if (nm_streq (obj->action.kind, NM_PLATFORM_ACTION_KIND_SIMPLE)) {
			nm_hash_update_strarr (h, obj->action.simple.sdata);
+		} else if (nm_streq (obj->action.kind, NM_PLATFORM_ACTION_KIND_MIRRED)) {
+			nm_hash_update_vals (h,
+			                     obj->action.mirred.ingress,
+			                     obj->action.mirred.egress,
+			                     obj->action.mirred.mirror,
+			                     obj->action.mirred.redirect,
+			                     obj->action.mirred.ifindex);
+		}
	}
 }

@@ -6579,8 +6595,15 @@ nm_platform_tfilter_cmp (const NMPlatformTfilter *a, const NMPlatformTfilter *b)

	NM_CMP_FIELD_STR_INTERNED (a, b, action.kind);
	if (a->action.kind) {
-		if (nm_streq (a->action.kind, NM_PLATFORM_ACTION_KIND_SIMPLE))
+		if (nm_streq (a->action.kind, NM_PLATFORM_ACTION_KIND_SIMPLE)) {
			NM_CMP_FIELD_STR (a, b, action.simple.sdata);
+		} else if (nm_streq (a->action.kind, NM_PLATFORM_ACTION_KIND_MIRRED)) {
+			NM_CMP_FIELD (a, b, action.mirred.ingress);
+			NM_CMP_FIELD (a, b, action.mirred.egress);
+			NM_CMP_FIELD (a, b, action.mirred.mirror);
+			NM_CMP_FIELD (a, b, action.mirred.redirect);
+			NM_CMP_FIELD (a, b, action.mirred.ifindex);
+		}
	}

	return 0;
diff --git a/src/platform/nm-platform.h b/src/platform/nm-platform.h
index 8742b90555..16747f093b 100644
--- a/src/platform/nm-platform.h
+++ b/src/platform/nm-platform.h
@@ -623,14 +623,24 @@ typedef struct {
	char sdata[32];
 } NMPlatformActionSimple;

+typedef struct {
+	gboolean egress;
+	gboolean ingress;
+	gboolean mirror;
+	gboolean redirect;
+	int ifindex;
+} NMPlatformActionMirred;
+
 typedef struct {
	const char *kind;
	union {
		NMPlatformActionSimple simple;
+		NMPlatformActionMirred mirred;
	};
 } NMPlatformAction;

 #define NM_PLATFORM_ACTION_KIND_SIMPLE "simple"
+#define NM_PLATFORM_ACTION_KIND_MIRRED "mirred"

 typedef struct {
	__NMPlatformObjWithIfindex_COMMON;
--
2.21.0

From 84bd35e4fadf0aa8244b2c683c60fdfc4b87cf6f Mon Sep 17 00:00:00 2001
From: Thomas Haller <thaller@redhat.com>
Date: Wed, 1 May 2019 11:36:02 +0200
Subject: [PATCH 03/20] shared: use nm_str_skip_leading_spaces() in
 _nm_utils_ascii_str_to_int64()

(cherry picked from commit 9d2623cceb8550fbe6becf5dde2e0cef152e1086)
---
 shared/nm-glib-aux/nm-shared-utils.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/shared/nm-glib-aux/nm-shared-utils.c b/shared/nm-glib-aux/nm-shared-utils.c
index cf08a77fde..fb945ef9fb 100644
--- a/shared/nm-glib-aux/nm-shared-utils.c
+++ b/shared/nm-glib-aux/nm-shared-utils.c
@@ -734,10 +734,7 @@ _nm_utils_ascii_str_to_int64 (const char *str, guint base, gint64 min, gint64 ma
	gint64 v;
	const char *s = NULL;

-	if (str) {
-		while (g_ascii_isspace (str[0]))
-			str++;
-	}
+	str = nm_str_skip_leading_spaces (str);
	if (!str || !str[0]) {
		errno = EINVAL;
		return fallback;
@@ -748,9 +745,9 @@ _nm_utils_ascii_str_to_int64 (const char *str, guint base, gint64 min, gint64 ma

	if (errno != 0)
		return fallback;
+
	if (s[0] != '\0') {
-		while (g_ascii_isspace (s[0]))
-			s++;
+		s = nm_str_skip_leading_spaces (s);
		if (s[0] != '\0') {
			errno = EINVAL;
			return fallback;
--
2.21.0

From 13e3bd4161d11c81cd4188a733c6d370d41452c3 Mon Sep 17 00:00:00 2001
From: Thomas Haller <thaller@redhat.com>
Date: Wed, 1 May 2019 10:49:16 +0200
Subject: [PATCH 04/20] libnm/tests: add test for _nm_utils_parse_tc_handle()

(cherry picked from commit fac95d0062d9bbe256b8e479ba7cb452cbac340e)
---
 libnm-core/tests/test-setting.c | 56 +++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/libnm-core/tests/test-setting.c b/libnm-core/tests/test-setting.c
index 03100a039b..7587b65ba7 100644
--- a/libnm-core/tests/test-setting.c
+++ b/libnm-core/tests/test-setting.c
@@ -2979,6 +2979,60 @@ test_routing_rule (gconstpointer test_data)

 /*****************************************************************************/

+static void
+test_parse_tc_handle (void)
+{
+#define _parse_tc_handle(str, exp) \
+	G_STMT_START { \
+		gs_free_error GError *_error = NULL; \
+		GError **_perror = nmtst_get_rand_bool () ? &_error : NULL; \
+		guint32 _v; \
+		const guint32 _v_exp = (exp); \
+		\
+		_v = _nm_utils_parse_tc_handle (""str"", _perror); \
+		\
+		if (_v != _v_exp) \
+			g_error ("%s:%d: \"%s\" gave %08x but %08x expected.", __FILE__, __LINE__, ""str"", _v, _v_exp); \
+		\
+		if (_v == TC_H_UNSPEC) \
+			g_assert (!_perror || *_perror); \
+		else \
+			g_assert (!_perror || !*_perror); \
+		\
+	} G_STMT_END
+
+#define _parse_tc_handle_inval(str)           _parse_tc_handle (str, TC_H_UNSPEC)
+#define _parse_tc_handle_valid(str, maj, min) _parse_tc_handle (str, TC_H_MAKE (((guint32) (maj)) << 16, ((guint16) (min))))
+
+	_parse_tc_handle_inval ("");
+	_parse_tc_handle_inval (" ");
+	_parse_tc_handle_inval (" \n");
+	_parse_tc_handle_valid ("1", 1, 0);
+	_parse_tc_handle_inval(" 1 ");
+	_parse_tc_handle_valid ("1:", 1, 0);
+	_parse_tc_handle_inval ("1:  ");
+	_parse_tc_handle_valid ("1:0", 1, 0);
+	_parse_tc_handle_inval ("1   :0");
+	_parse_tc_handle_inval ("1   \t\n\f\r:0");
+	_parse_tc_handle_inval ("1   \t\n\f\r\v:0");
+	_parse_tc_handle_inval (" 1 : 0  ");
+	_parse_tc_handle_valid (" \t\v\n1: 0", 1, 0);
+	_parse_tc_handle_valid ("1:2", 1, 2);
+	_parse_tc_handle_valid ("01:02", 1, 2);
+	_parse_tc_handle_valid ("0x01:0x02", 1, 2);
+	_parse_tc_handle_valid ("  01:   02", 1, 2);
+	_parse_tc_handle_valid ("019:   020", 0x19, 0x20);
+	_parse_tc_handle_valid ("FFFF:   020", 0xFFFF, 0x20);
+	_parse_tc_handle_valid ("FfFF:   ffff", 0xFFFF, 0xFFFF);
+	_parse_tc_handle_valid ("FFFF", 0xFFFF, 0);
+	_parse_tc_handle_valid ("0xFFFF", 0xFFFF, 0);
+	_parse_tc_handle_inval ("10000");
+	_parse_tc_handle_valid ("\t\n\f\r FFFF", 0xFFFF, 0);
+	_parse_tc_handle_valid ("\t\n\f\r \vFFFF", 0xFFFF, 0);
+}
+
+/*****************************************************************************/
+
 NMTST_DEFINE ();

 int
@@ -3064,5 +3118,7 @@ main (int argc, char **argv)

	g_test_add_data_func ("/libnm/settings/routing-rule/1", GINT_TO_POINTER (0), test_routing_rule);

+	g_test_add_func ("/libnm/parse-tc-handle", test_parse_tc_handle);
+
	return g_test_run ();
 }
--
2.21.0

From b954ddc2752285b28885398441879edb922c68fc Mon Sep 17 00:00:00 2001
From: Thomas Haller <thaller@redhat.com>
Date: Wed, 1 May 2019 10:27:32 +0200
Subject: [PATCH 05/20] libnm: cleanup _nm_utils_parse_tc_handle()

- g_ascii_strtoll() accepts leading spaces, but it leaves
  the end pointer at the first space after the digit. That means,
  we accepted "1: 0" but not "1 :0". We should either consistently
  accept spaces around the digits/colon or reject it.

- g_ascii_strtoll() accepts "\v" as a space (just like `man 3 isspace`
  comments that "\v" is a space in C and POSIX locale.
  For some reasons (unknown to me) g_ascii_isspace() does not treat
  "\v" as space. And neither does NM_ASCII_SPACES and
  nm_str_skip_leading_spaces().
  We should be consistent about what we consider spaces and what not.
  It's already odd to accept '\n' as spaces here, but well, lets do
  it for the sake of consistency (so that it matches with our
  understanding of ASCII spaces, albeit not POSIX's).

- don't use bogus error domains in "g_set_error (error, 1, 0, ..."
  That is a bug and we have NM_UTILS_ERROR exactly for error instances
  with unspecified domain and code.

- as before, accept a trailing ":" with omitted minor number.

- reject all unexpected characters. strtoll() accepts '+' / '-'
  and a "0x" prefix of the numbers (and leading POSIX spaces). Be
  strict here and only accepts NM_ASCII_SPACES, ':', and hexdigits.
  In particular, don't accept the "0x" prefix.

This parsing would be significantly simpler to implement, if we could
just strdup() the string, split the string at the colon delimiter and
use _nm_utils_ascii_str_to_int64() which gets leading/trailing spaces
right. But let's save the "overhead" of an additional alloc.

(cherry picked from commit cc9f07167607124bcb0df735034858aadfdb8541)
---
 libnm-core/nm-utils.c           | 43 ++++++++++++++++++++++++---------
 libnm-core/tests/test-setting.c | 18 +++++++-------
 2 files changed, 41 insertions(+), 20 deletions(-)

diff --git a/libnm-core/nm-utils.c b/libnm-core/nm-utils.c
index 3af3e04ed9..7a7c6de114 100644
--- a/libnm-core/nm-utils.c
+++ b/libnm-core/nm-utils.c
@@ -2280,21 +2280,42 @@ _nm_utils_string_append_tc_parent (GString *string, const char *prefix, guint32
 guint32
 _nm_utils_parse_tc_handle (const char *str, GError **error)
 {
-	gint64 maj, min;
-	char *sep;
+	gint64 maj;
+	gint64 min = 0;
+	const char *sep;

-	maj = g_ascii_strtoll (str, &sep, 0x10);
-	if (*sep == ':')
-		min = g_ascii_strtoll (&sep[1], &sep, 0x10);
-	else
-		min = 0;
+	nm_assert (str);

-	if (*sep != '\0' || maj <= 0 || maj > 0xffff || min < 0 || min > 0xffff) {
-		g_set_error (error, 1, 0, _("'%s' is not a valid handle."), str);
-		return TC_H_UNSPEC;
+	maj = g_ascii_strtoll (str, (char **) &sep, 0x10);
+	if (sep == str)
+		goto fail;
+
+	sep = nm_str_skip_leading_spaces (sep);
+
+	if (sep[0] == ':') {
+		const char *str2 = &sep[1];
+
+		min = g_ascii_strtoll (str2, (char **) &sep, 0x10);
+		sep = nm_str_skip_leading_spaces (sep);
+		if (sep[0] != '\0')
+			goto fail;
+	} else if (sep[0] != '\0')
+		goto fail;
+
+	if (   maj <= 0
+	    || maj > 0xffff
+	    || min < 0
+	    || min > 0xffff
+	    || !NM_STRCHAR_ALL (str, ch, (   g_ascii_isxdigit (ch)
+	                                  || ch == ':'
+	                                  || g_ascii_isspace (ch)))) {
+		goto fail;
	}

-	return TC_H_MAKE (maj << 16, min);
+	return TC_H_MAKE (((guint32) maj) << 16, (guint32) min);
+fail:
+	nm_utils_error_set (error, NM_UTILS_ERROR_UNKNOWN, _("'%s' is not a valid handle."), str);
+	return TC_H_UNSPEC;
 }

 #define TC_ATTR_SPEC_PTR(name, type, no_value, consumes_rest, str_type) \
diff --git a/libnm-core/tests/test-setting.c b/libnm-core/tests/test-setting.c
index 7587b65ba7..5c9c614fb5 100644
--- a/libnm-core/tests/test-setting.c
+++ b/libnm-core/tests/test-setting.c
@@ -3008,27 +3008,27 @@ test_parse_tc_handle (void)
	_parse_tc_handle_inval (" ");
	_parse_tc_handle_inval (" \n");
	_parse_tc_handle_valid ("1", 1, 0);
-	_parse_tc_handle_inval(" 1 ");
+	_parse_tc_handle_valid(" 1 ", 1, 0);
	_parse_tc_handle_valid ("1:", 1, 0);
-	_parse_tc_handle_inval ("1:  ");
+	_parse_tc_handle_valid ("1:  ", 1, 0);
	_parse_tc_handle_valid ("1:0", 1, 0);
-	_parse_tc_handle_inval ("1   :0");
-	_parse_tc_handle_inval ("1   \t\n\f\r:0");
+	_parse_tc_handle_valid ("1   :0", 1, 0);
+	_parse_tc_handle_valid ("1   \t\n\f\r:0", 1, 0);
	_parse_tc_handle_inval ("1   \t\n\f\r\v:0");
-	_parse_tc_handle_inval (" 1 : 0  ");
-	_parse_tc_handle_valid (" \t\v\n1: 0", 1, 0);
+	_parse_tc_handle_valid (" 1 : 0  ", 1, 0);
+	_parse_tc_handle_inval (" \t\v\n1: 0");
	_parse_tc_handle_valid ("1:2", 1, 2);
	_parse_tc_handle_valid ("01:02", 1, 2);
-	_parse_tc_handle_valid ("0x01:0x02", 1, 2);
+	_parse_tc_handle_inval ("0x01:0x02");
	_parse_tc_handle_valid ("  01:   02", 1, 2);
	_parse_tc_handle_valid ("019:   020", 0x19, 0x20);
	_parse_tc_handle_valid ("FFFF:   020", 0xFFFF, 0x20);
	_parse_tc_handle_valid ("FfFF:   ffff", 0xFFFF, 0xFFFF);
	_parse_tc_handle_valid ("FFFF", 0xFFFF, 0);
-	_parse_tc_handle_valid ("0xFFFF", 0xFFFF, 0);
+	_parse_tc_handle_inval ("0xFFFF");
	_parse_tc_handle_inval ("10000");
	_parse_tc_handle_valid ("\t\n\f\r FFFF", 0xFFFF, 0);
-	_parse_tc_handle_valid ("\t\n\f\r \vFFFF", 0xFFFF, 0);
+	_parse_tc_handle_inval ("\t\n\f\r \vFFFF");
 }

 /*****************************************************************************/
--
2.21.0

From fde9250cdc664b557a80a517f57929a36597094a Mon Sep 17 00:00:00 2001
From: Thomas Haller <thaller@redhat.com>
Date: Wed, 1 May 2019 11:53:13 +0200
Subject: [PATCH 06/20] libnm: mark NMVariantAttributeSpec pointers as const

This actually allows the compiler/linker to mark the memory as read-only and any
modification will cause a segmentation fault.

I would also think that it allows the compiler to put the structure directly
beside the outer constant array (in which this pointer is embedded). That is good
locality-wise.

(cherry picked from commit 4e3955e6ddf02d5ea32012bd563aa02fece5c0ef)
---
 libnm-core/nm-setting-ip-config.c | 2 +-
 libnm-core/nm-setting-sriov.c     | 2 +-
 libnm-core/nm-utils.c             | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/libnm-core/nm-setting-ip-config.c b/libnm-core/nm-setting-ip-config.c
index f362945f41..26fbc8849f 100644
--- a/libnm-core/nm-setting-ip-config.c
+++ b/libnm-core/nm-setting-ip-config.c
@@ -1213,7 +1213,7 @@ nm_ip_route_set_attribute (NMIPRoute *route, const char *name, GVariant *value)
 }

 #define ATTR_SPEC_PTR(name, type, v4, v6, str_type) \
-	&(NMVariantAttributeSpec) { name, type, v4, v6, FALSE, FALSE, str_type }
+	&((const NMVariantAttributeSpec) { name, type, v4, v6, FALSE, FALSE, str_type })

 static const NMVariantAttributeSpec * const ip_route_attribute_spec[] = {
	ATTR_SPEC_PTR (NM_IP_ROUTE_ATTRIBUTE_TABLE,           G_VARIANT_TYPE_UINT32,   TRUE,  TRUE,  0 ),
diff --git a/libnm-core/nm-setting-sriov.c b/libnm-core/nm-setting-sriov.c
index b662ca2cf6..9a47d141d2 100644
--- a/libnm-core/nm-setting-sriov.c
+++ b/libnm-core/nm-setting-sriov.c
@@ -366,7 +366,7 @@ nm_sriov_vf_get_attribute (const NMSriovVF *vf, const char *name)
 }

 #define SRIOV_ATTR_SPEC_PTR(name, type, str_type) \
-	&(NMVariantAttributeSpec) { name, type, FALSE, FALSE, FALSE, FALSE, str_type }
+	&((const NMVariantAttributeSpec) { name, type, FALSE, FALSE, FALSE, FALSE, str_type })

 const NMVariantAttributeSpec * const _nm_sriov_vf_attribute_spec[] = {
	SRIOV_ATTR_SPEC_PTR (NM_SRIOV_VF_ATTRIBUTE_MAC,          G_VARIANT_TYPE_STRING,  'm'),
diff --git a/libnm-core/nm-utils.c b/libnm-core/nm-utils.c
index 7a7c6de114..7df63b83bb 100644
--- a/libnm-core/nm-utils.c
+++ b/libnm-core/nm-utils.c
@@ -2319,7 +2319,7 @@ fail:
 }

 #define TC_ATTR_SPEC_PTR(name, type, no_value, consumes_rest, str_type) \
-	&(NMVariantAttributeSpec) { name, type, FALSE, FALSE, no_value, consumes_rest, str_type }
+	&((const NMVariantAttributeSpec) { name, type, FALSE, FALSE, no_value, consumes_rest, str_type })

 static const NMVariantAttributeSpec * const tc_object_attribute_spec[] = {
	TC_ATTR_SPEC_PTR ("root",    G_VARIANT_TYPE_BOOLEAN, TRUE,  FALSE, 0   ),
--
2.21.0

From 38cf36022ef0bb678daf4d2e70da82c96185b310 Mon Sep 17 00:00:00 2001
From: Thomas Haller <thaller@redhat.com>
Date: Wed, 1 May 2019 11:56:29 +0200
Subject: [PATCH 07/20] libnm: use macro and designated initializers for
 NMVariantAttributeSpec

I think initializing structs should (almost) be always done with designated
initializers, because otherwise it's easy to get the order wrong. The
problem is that otherwise the order of fields gets additional meaning
not only for the memory layout, but also for the code that initialize
the structs.

Add a macro NM_VARIANT_ATTRIBUTE_SPEC_DEFINE() that replaces the other
(duplicate) macros. This macro also gets it right to mark the struct as
const.

(cherry picked from commit 86dc50d4760b77f30f3d29c5fa46ea32b06d73f8)
---
 libnm-core/nm-setting-ip-config.c | 35 ++++++++--------
 libnm-core/nm-setting-sriov.c     | 17 ++++----
 libnm-core/nm-utils-private.h     |  7 ++++
 libnm-core/nm-utils.c             | 67 +++++++++++++++----------------
 4 files changed, 62 insertions(+), 64 deletions(-)

diff --git a/libnm-core/nm-setting-ip-config.c b/libnm-core/nm-setting-ip-config.c
index 26fbc8849f..375c309dd6 100644
--- a/libnm-core/nm-setting-ip-config.c
+++ b/libnm-core/nm-setting-ip-config.c
@@ -1212,25 +1212,22 @@ nm_ip_route_set_attribute (NMIPRoute *route, const char *name, GVariant *value)
		g_hash_table_remove (route->attributes, name);
 }

-#define ATTR_SPEC_PTR(name, type, v4, v6, str_type) \
-	&((const NMVariantAttributeSpec) { name, type, v4, v6, FALSE, FALSE, str_type })
-
-static const NMVariantAttributeSpec * const ip_route_attribute_spec[] = {
-	ATTR_SPEC_PTR (NM_IP_ROUTE_ATTRIBUTE_TABLE,           G_VARIANT_TYPE_UINT32,   TRUE,  TRUE,  0 ),
-	ATTR_SPEC_PTR (NM_IP_ROUTE_ATTRIBUTE_SRC,             G_VARIANT_TYPE_STRING,   TRUE,  TRUE, 'a'),
-	ATTR_SPEC_PTR (NM_IP_ROUTE_ATTRIBUTE_FROM,            G_VARIANT_TYPE_STRING,   FALSE, TRUE, 'p'),
-	ATTR_SPEC_PTR (NM_IP_ROUTE_ATTRIBUTE_TOS,             G_VARIANT_TYPE_BYTE,     TRUE,  FALSE, 0 ),
-	ATTR_SPEC_PTR (NM_IP_ROUTE_ATTRIBUTE_ONLINK,          G_VARIANT_TYPE_BOOLEAN,  TRUE,  TRUE,  0 ),
-	ATTR_SPEC_PTR (NM_IP_ROUTE_ATTRIBUTE_WINDOW,          G_VARIANT_TYPE_UINT32,   TRUE,  TRUE,  0 ),
-	ATTR_SPEC_PTR (NM_IP_ROUTE_ATTRIBUTE_CWND,            G_VARIANT_TYPE_UINT32,   TRUE,  TRUE,  0 ),
-	ATTR_SPEC_PTR (NM_IP_ROUTE_ATTRIBUTE_INITCWND,        G_VARIANT_TYPE_UINT32,   TRUE,  TRUE,  0 ),
-	ATTR_SPEC_PTR (NM_IP_ROUTE_ATTRIBUTE_INITRWND,        G_VARIANT_TYPE_UINT32,   TRUE,  TRUE,  0 ),
-	ATTR_SPEC_PTR (NM_IP_ROUTE_ATTRIBUTE_MTU,             G_VARIANT_TYPE_UINT32,   TRUE,  TRUE,  0 ),
-	ATTR_SPEC_PTR (NM_IP_ROUTE_ATTRIBUTE_LOCK_WINDOW,     G_VARIANT_TYPE_BOOLEAN,  TRUE,  TRUE,  0 ),
-	ATTR_SPEC_PTR (NM_IP_ROUTE_ATTRIBUTE_LOCK_CWND,       G_VARIANT_TYPE_BOOLEAN,  TRUE,  TRUE,  0 ),
-	ATTR_SPEC_PTR (NM_IP_ROUTE_ATTRIBUTE_LOCK_INITCWND,   G_VARIANT_TYPE_BOOLEAN,  TRUE,  TRUE,  0 ),
-	ATTR_SPEC_PTR (NM_IP_ROUTE_ATTRIBUTE_LOCK_INITRWND,   G_VARIANT_TYPE_BOOLEAN,  TRUE,  TRUE,  0 ),
-	ATTR_SPEC_PTR (NM_IP_ROUTE_ATTRIBUTE_LOCK_MTU,        G_VARIANT_TYPE_BOOLEAN,  TRUE,  TRUE,  0 ),
+static const NMVariantAttributeSpec *const ip_route_attribute_spec[] = {
+	NM_VARIANT_ATTRIBUTE_SPEC_DEFINE (NM_IP_ROUTE_ATTRIBUTE_TABLE,         G_VARIANT_TYPE_UINT32,  .v4 = TRUE, .v6 = TRUE,                  ),
+	NM_VARIANT_ATTRIBUTE_SPEC_DEFINE (NM_IP_ROUTE_ATTRIBUTE_SRC,           G_VARIANT_TYPE_STRING,  .v4 = TRUE, .v6 = TRUE, .str_type = 'a', ),
+	NM_VARIANT_ATTRIBUTE_SPEC_DEFINE (NM_IP_ROUTE_ATTRIBUTE_FROM,          G_VARIANT_TYPE_STRING,              .v6 = TRUE, .str_type = 'p', ),
+	NM_VARIANT_ATTRIBUTE_SPEC_DEFINE (NM_IP_ROUTE_ATTRIBUTE_TOS,           G_VARIANT_TYPE_BYTE,    .v4 = TRUE,                              ),
+	NM_VARIANT_ATTRIBUTE_SPEC_DEFINE (NM_IP_ROUTE_ATTRIBUTE_ONLINK,        G_VARIANT_TYPE_BOOLEAN, .v4 = TRUE, .v6 = TRUE,                  ),
+	NM_VARIANT_ATTRIBUTE_SPEC_DEFINE (NM_IP_ROUTE_ATTRIBUTE_WINDOW,        G_VARIANT_TYPE_UINT32,  .v4 = TRUE, .v6 = TRUE,                  ),
+	NM_VARIANT_ATTRIBUTE_SPEC_DEFINE (NM_IP_ROUTE_ATTRIBUTE_CWND,          G_VARIANT_TYPE_UINT32,  .v4 = TRUE, .v6 = TRUE,                  ),
+	NM_VARIANT_ATTRIBUTE_SPEC_DEFINE (NM_IP_ROUTE_ATTRIBUTE_INITCWND,      G_VARIANT_TYPE_UINT32,  .v4 = TRUE, .v6 = TRUE,                  ),
+	NM_VARIANT_ATTRIBUTE_SPEC_DEFINE (NM_IP_ROUTE_ATTRIBUTE_INITRWND,      G_VARIANT_TYPE_UINT32,  .v4 = TRUE, .v6 = TRUE,                  ),
+	NM_VARIANT_ATTRIBUTE_SPEC_DEFINE (NM_IP_ROUTE_ATTRIBUTE_MTU,           G_VARIANT_TYPE_UINT32,  .v4 = TRUE, .v6 = TRUE,                  ),
+	NM_VARIANT_ATTRIBUTE_SPEC_DEFINE (NM_IP_ROUTE_ATTRIBUTE_LOCK_WINDOW,   G_VARIANT_TYPE_BOOLEAN, .v4 = TRUE, .v6 = TRUE,                  ),
+	NM_VARIANT_ATTRIBUTE_SPEC_DEFINE (NM_IP_ROUTE_ATTRIBUTE_LOCK_CWND,     G_VARIANT_TYPE_BOOLEAN, .v4 = TRUE, .v6 = TRUE,                  ),
+	NM_VARIANT_ATTRIBUTE_SPEC_DEFINE (NM_IP_ROUTE_ATTRIBUTE_LOCK_INITCWND, G_VARIANT_TYPE_BOOLEAN, .v4 = TRUE, .v6 = TRUE,                  ),
+	NM_VARIANT_ATTRIBUTE_SPEC_DEFINE (NM_IP_ROUTE_ATTRIBUTE_LOCK_INITRWND, G_VARIANT_TYPE_BOOLEAN, .v4 = TRUE, .v6 = TRUE,                  ),
+	NM_VARIANT_ATTRIBUTE_SPEC_DEFINE (NM_IP_ROUTE_ATTRIBUTE_LOCK_MTU,      G_VARIANT_TYPE_BOOLEAN, .v4 = TRUE, .v6 = TRUE,                  ),
	NULL,
 };

diff --git a/libnm-core/nm-setting-sriov.c b/libnm-core/nm-setting-sriov.c
index 9a47d141d2..0625331e3f 100644
--- a/libnm-core/nm-setting-sriov.c
+++ b/libnm-core/nm-setting-sriov.c
@@ -365,17 +365,14 @@ nm_sriov_vf_get_attribute (const NMSriovVF *vf, const char *name)
	return g_hash_table_lookup (vf->attributes, name);
 }

-#define SRIOV_ATTR_SPEC_PTR(name, type, str_type) \
-	&((const NMVariantAttributeSpec) { name, type, FALSE, FALSE, FALSE, FALSE, str_type })
-
-const NMVariantAttributeSpec * const _nm_sriov_vf_attribute_spec[] = {
-	SRIOV_ATTR_SPEC_PTR (NM_SRIOV_VF_ATTRIBUTE_MAC,          G_VARIANT_TYPE_STRING,  'm'),
-	SRIOV_ATTR_SPEC_PTR (NM_SRIOV_VF_ATTRIBUTE_SPOOF_CHECK,  G_VARIANT_TYPE_BOOLEAN,  0),
-	SRIOV_ATTR_SPEC_PTR (NM_SRIOV_VF_ATTRIBUTE_TRUST,        G_VARIANT_TYPE_BOOLEAN,  0),
-	SRIOV_ATTR_SPEC_PTR (NM_SRIOV_VF_ATTRIBUTE_MIN_TX_RATE,  G_VARIANT_TYPE_UINT32,   0),
-	SRIOV_ATTR_SPEC_PTR (NM_SRIOV_VF_ATTRIBUTE_MAX_TX_RATE,  G_VARIANT_TYPE_UINT32,   0),
+const NMVariantAttributeSpec *const _nm_sriov_vf_attribute_spec[] = {
+	NM_VARIANT_ATTRIBUTE_SPEC_DEFINE (NM_SRIOV_VF_ATTRIBUTE_MAC,         G_VARIANT_TYPE_STRING,  .str_type = 'm', ),
+	NM_VARIANT_ATTRIBUTE_SPEC_DEFINE (NM_SRIOV_VF_ATTRIBUTE_SPOOF_CHECK, G_VARIANT_TYPE_BOOLEAN,                  ),
+	NM_VARIANT_ATTRIBUTE_SPEC_DEFINE (NM_SRIOV_VF_ATTRIBUTE_TRUST,       G_VARIANT_TYPE_BOOLEAN,                  ),
+	NM_VARIANT_ATTRIBUTE_SPEC_DEFINE (NM_SRIOV_VF_ATTRIBUTE_MIN_TX_RATE, G_VARIANT_TYPE_UINT32,                   ),
+	NM_VARIANT_ATTRIBUTE_SPEC_DEFINE (NM_SRIOV_VF_ATTRIBUTE_MAX_TX_RATE, G_VARIANT_TYPE_UINT32,                   ),
	/* D-Bus only, synthetic attributes */
-	SRIOV_ATTR_SPEC_PTR ("vlans",                            G_VARIANT_TYPE_STRING,  'd'),
+	NM_VARIANT_ATTRIBUTE_SPEC_DEFINE ("vlans",                           G_VARIANT_TYPE_STRING,  .str_type = 'd', ),
	NULL,
 };

diff --git a/libnm-core/nm-utils-private.h b/libnm-core/nm-utils-private.h
index a1a1369a39..1b9b888773 100644
--- a/libnm-core/nm-utils-private.h
+++ b/libnm-core/nm-utils-private.h
@@ -38,6 +38,13 @@ struct _NMVariantAttributeSpec {
	char str_type;
 };

+#define NM_VARIANT_ATTRIBUTE_SPEC_DEFINE(_name, _type, ...) \
+	(&((const NMVariantAttributeSpec) { \
+		.name          = _name, \
+		.type          = _type, \
+		__VA_ARGS__ \
+	}))
+
 gboolean    _nm_utils_string_slist_validate (GSList *list,
                                              const char **valid_values);

diff --git a/libnm-core/nm-utils.c b/libnm-core/nm-utils.c
index 7df63b83bb..7cf482d3de 100644
--- a/libnm-core/nm-utils.c
+++ b/libnm-core/nm-utils.c
@@ -2318,33 +2318,30 @@ fail:
	return TC_H_UNSPEC;
 }

-#define TC_ATTR_SPEC_PTR(name, type, no_value, consumes_rest, str_type) \
-	&((const NMVariantAttributeSpec) { name, type, FALSE, FALSE, no_value, consumes_rest, str_type })
-
-static const NMVariantAttributeSpec * const tc_object_attribute_spec[] = {
-	TC_ATTR_SPEC_PTR ("root",    G_VARIANT_TYPE_BOOLEAN, TRUE,  FALSE, 0   ),
-	TC_ATTR_SPEC_PTR ("parent",  G_VARIANT_TYPE_STRING,  FALSE, FALSE, 'a' ),
-	TC_ATTR_SPEC_PTR ("handle",  G_VARIANT_TYPE_STRING,  FALSE, FALSE, 'a' ),
-	TC_ATTR_SPEC_PTR ("kind",    G_VARIANT_TYPE_STRING,  TRUE,  FALSE, 'a' ),
-	TC_ATTR_SPEC_PTR ("",        G_VARIANT_TYPE_STRING,  TRUE,  TRUE,  'a' ),
+static const NMVariantAttributeSpec *const tc_object_attribute_spec[] = {
+	NM_VARIANT_ATTRIBUTE_SPEC_DEFINE ("root",   G_VARIANT_TYPE_BOOLEAN, .no_value = TRUE,                                         ),
+	NM_VARIANT_ATTRIBUTE_SPEC_DEFINE ("parent", G_VARIANT_TYPE_STRING,                                           .str_type = 'a', ),
+	NM_VARIANT_ATTRIBUTE_SPEC_DEFINE ("handle", G_VARIANT_TYPE_STRING,                                           .str_type = 'a', ),
+	NM_VARIANT_ATTRIBUTE_SPEC_DEFINE ("kind",   G_VARIANT_TYPE_STRING,  .no_value = TRUE,                        .str_type = 'a', ),
+	NM_VARIANT_ATTRIBUTE_SPEC_DEFINE ("",       G_VARIANT_TYPE_STRING,  .no_value = TRUE, .consumes_rest = TRUE, .str_type = 'a', ),
	NULL,
 };

-static const NMVariantAttributeSpec * const tc_qdisc_fq_codel_spec[] = {
-	TC_ATTR_SPEC_PTR ("limit",        G_VARIANT_TYPE_UINT32,  FALSE, FALSE, 0   ),
-	TC_ATTR_SPEC_PTR ("flows",        G_VARIANT_TYPE_UINT32,  FALSE, FALSE, 0   ),
-	TC_ATTR_SPEC_PTR ("target",       G_VARIANT_TYPE_UINT32,  FALSE, FALSE, 0   ),
-	TC_ATTR_SPEC_PTR ("interval",     G_VARIANT_TYPE_UINT32,  FALSE, FALSE, 0   ),
-	TC_ATTR_SPEC_PTR ("quantum",      G_VARIANT_TYPE_UINT32,  FALSE, FALSE, 0   ),
-	TC_ATTR_SPEC_PTR ("ce_threshold", G_VARIANT_TYPE_UINT32,  FALSE, FALSE, 0   ),
-	TC_ATTR_SPEC_PTR ("memory",       G_VARIANT_TYPE_UINT32,  FALSE, FALSE, 0   ),
-	TC_ATTR_SPEC_PTR ("ecn",          G_VARIANT_TYPE_BOOLEAN, TRUE,  FALSE, 0   ),
+static const NMVariantAttributeSpec *const tc_qdisc_fq_codel_spec[] = {
+	NM_VARIANT_ATTRIBUTE_SPEC_DEFINE ("limit",        G_VARIANT_TYPE_UINT32,                    ),
+	NM_VARIANT_ATTRIBUTE_SPEC_DEFINE ("flows",        G_VARIANT_TYPE_UINT32,                    ),
+	NM_VARIANT_ATTRIBUTE_SPEC_DEFINE ("target",       G_VARIANT_TYPE_UINT32,                    ),
+	NM_VARIANT_ATTRIBUTE_SPEC_DEFINE ("interval",     G_VARIANT_TYPE_UINT32,                    ),
+	NM_VARIANT_ATTRIBUTE_SPEC_DEFINE ("quantum",      G_VARIANT_TYPE_UINT32,                    ),
+	NM_VARIANT_ATTRIBUTE_SPEC_DEFINE ("ce_threshold", G_VARIANT_TYPE_UINT32,                    ),
+	NM_VARIANT_ATTRIBUTE_SPEC_DEFINE ("memory",       G_VARIANT_TYPE_UINT32,                    ),
+	NM_VARIANT_ATTRIBUTE_SPEC_DEFINE ("ecn",          G_VARIANT_TYPE_BOOLEAN, .no_value = TRUE, ),
	NULL,
 };

 typedef struct {
	const char *kind;
-	const NMVariantAttributeSpec * const *attrs;
+	const NMVariantAttributeSpec *const *attrs;
 } NMQdiscAttributeSpec;

 static const NMQdiscAttributeSpec *const tc_qdisc_attribute_spec[] = {
@@ -2558,23 +2555,23 @@ nm_utils_tc_qdisc_from_str (const char *str, GError **error)

 /*****************************************************************************/

-static const NMVariantAttributeSpec * const tc_action_simple_attribute_spec[] = {
-	TC_ATTR_SPEC_PTR ("sdata",   G_VARIANT_TYPE_BYTESTRING,  FALSE, FALSE, 0   ),
+static const NMVariantAttributeSpec *const tc_action_simple_attribute_spec[] = {
+	NM_VARIANT_ATTRIBUTE_SPEC_DEFINE ("sdata", G_VARIANT_TYPE_BYTESTRING, ),
	NULL,
 };

-static const NMVariantAttributeSpec * const tc_action_mirred_attribute_spec[] = {
-	TC_ATTR_SPEC_PTR ("egress",   G_VARIANT_TYPE_BOOLEAN, TRUE,  FALSE, 0   ),
-	TC_ATTR_SPEC_PTR ("ingress",  G_VARIANT_TYPE_BOOLEAN, TRUE,  FALSE, 0   ),
-	TC_ATTR_SPEC_PTR ("mirror",   G_VARIANT_TYPE_BOOLEAN, TRUE,  FALSE, 0   ),
-	TC_ATTR_SPEC_PTR ("redirect", G_VARIANT_TYPE_BOOLEAN, TRUE,  FALSE, 0   ),
-	TC_ATTR_SPEC_PTR ("dev",      G_VARIANT_TYPE_STRING,  TRUE,  FALSE, 'a' ),
+static const NMVariantAttributeSpec *const tc_action_mirred_attribute_spec[] = {
+	NM_VARIANT_ATTRIBUTE_SPEC_DEFINE ("egress",   G_VARIANT_TYPE_BOOLEAN, .no_value = TRUE,                  ),
+	NM_VARIANT_ATTRIBUTE_SPEC_DEFINE ("ingress",  G_VARIANT_TYPE_BOOLEAN, .no_value = TRUE,                  ),
+	NM_VARIANT_ATTRIBUTE_SPEC_DEFINE ("mirror",   G_VARIANT_TYPE_BOOLEAN, .no_value = TRUE,                  ),
+	NM_VARIANT_ATTRIBUTE_SPEC_DEFINE ("redirect", G_VARIANT_TYPE_BOOLEAN, .no_value = TRUE,                  ),
+	NM_VARIANT_ATTRIBUTE_SPEC_DEFINE ("dev",      G_VARIANT_TYPE_STRING,  .no_value = TRUE, .str_type = 'a', ),
	NULL,
 };

-static const NMVariantAttributeSpec * const tc_action_attribute_spec[] = {
-	TC_ATTR_SPEC_PTR ("kind",    G_VARIANT_TYPE_STRING,      TRUE,  FALSE, 'a' ),
-	TC_ATTR_SPEC_PTR ("",        G_VARIANT_TYPE_STRING,      TRUE,  TRUE,  'a' ),
+static const NMVariantAttributeSpec *const tc_action_attribute_spec[] = {
+	NM_VARIANT_ATTRIBUTE_SPEC_DEFINE ("kind",    G_VARIANT_TYPE_STRING, .no_value = TRUE,                        .str_type = 'a', ),
+	NM_VARIANT_ATTRIBUTE_SPEC_DEFINE ("",        G_VARIANT_TYPE_STRING, .no_value = TRUE, .consumes_rest = TRUE, .str_type = 'a', ),
	NULL,
 };

@@ -2643,7 +2640,7 @@ nm_utils_tc_action_from_str (const char *str, GError **error)
	gs_unref_hashtable GHashTable *ht = NULL;
	gs_unref_hashtable GHashTable *options = NULL;
	GVariant *variant;
-	const NMVariantAttributeSpec * const *attrs;
+	const NMVariantAttributeSpec *const *attrs;

	nm_assert (str);
	nm_assert (!error || !*error);
@@ -2771,9 +2768,9 @@ nm_utils_tc_tfilter_to_str (NMTCTfilter *tfilter, GError **error)
	return g_string_free (string, FALSE);
 }

-static const NMVariantAttributeSpec * const tc_tfilter_attribute_spec[] = {
-	TC_ATTR_SPEC_PTR ("action",  G_VARIANT_TYPE_BOOLEAN,     TRUE,  FALSE, 0   ),
-	TC_ATTR_SPEC_PTR ("",        G_VARIANT_TYPE_STRING,      TRUE,  TRUE,  'a' ),
+static const NMVariantAttributeSpec *const tc_tfilter_attribute_spec[] = {
+	NM_VARIANT_ATTRIBUTE_SPEC_DEFINE ("action", G_VARIANT_TYPE_BOOLEAN, .no_value = TRUE,                                         ),
+	NM_VARIANT_ATTRIBUTE_SPEC_DEFINE ("",       G_VARIANT_TYPE_STRING,  .no_value = TRUE, .consumes_rest = TRUE, .str_type = 'a', ),
	NULL,
 };

@@ -6460,7 +6457,7 @@ nm_utils_parse_variant_attributes (const char *string,
	gs_unref_hashtable GHashTable *ht = NULL;
	const char *ptr = string, *start = NULL, *sep;
	GVariant *variant;
-	const NMVariantAttributeSpec * const *s;
+	const NMVariantAttributeSpec *const *s;

	g_return_val_if_fail (string, NULL);
	g_return_val_if_fail (attr_separator, NULL);
--
2.21.0

From 73fdcd38f16eb759de395de8680c994a742fbe1c Mon Sep 17 00:00:00 2001
From: Thomas Haller <thaller@redhat.com>
Date: Wed, 1 May 2019 08:52:54 +0200
Subject: [PATCH 08/20] device: fix type of loop variable in tc_commit()

nqdiscs and ntfilters are unsigned integers. The loop variable must agree in
range and signedness.

(cherry picked from commit 438855e915c328867b7802b14ebef47d7f946ca8)
---
 src/devices/nm-device.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c
index ea6ad7e2ad..e07798094f 100644
--- a/src/devices/nm-device.c
+++ b/src/devices/nm-device.c
@@ -6496,7 +6496,7 @@ tc_commit (NMDevice *self)
	NMSettingTCConfig *s_tc = NULL;
	int ip_ifindex;
	guint nqdiscs, ntfilters;
-	int i;
+	guint i;

	connection = nm_device_get_applied_connection (self);
	if (connection)
--
2.21.0

From 366d3af00960803c973a307a6bd7ffeedd9c1520 Mon Sep 17 00:00:00 2001
From: Thomas Haller <thaller@redhat.com>
Date: Wed, 1 May 2019 08:10:47 +0200
Subject: [PATCH 09/20] platform: use NM_CMP_FIELD_UNSAFE() for comparing
 bitfield in nm_platform_qdisc_cmp()

"NM_CMP_FIELD (a, b, fq_codel.ecn == TRUE)" is quite a hack as it relies on
the implementation of the macro in a particular way. The problem is, that
NM_CMP_FIELD() uses typeof() which cannot be used with bitfields. So, the
nicer solution is to use NM_CMP_FIELD_UNSAFE() which exists exactly for bitfields
(it's "unsafe", because it evaluates arguments more than once as it avoids
the temporary variable with typeof()).

Same with nm_hash_update_vals() which uses typeof() to avoid evaluating
arguments more than once. But that again does not work with bitfields.
The "proper" way is to use NM_HASH_COMBINE_BOOLS().

(cherry picked from commit 47d8bee1130c619a41fe64753d88d88012f9fb98)
---
 src/platform/nm-platform.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/platform/nm-platform.c b/src/platform/nm-platform.c
index 6f23ddb589..f8cf0b8999 100644
--- a/src/platform/nm-platform.c
+++ b/src/platform/nm-platform.c
@@ -6480,7 +6480,8 @@ nm_platform_qdisc_hash_update (const NMPlatformQdisc *obj, NMHashState *h)
		                     obj->fq_codel.quantum,
		                     obj->fq_codel.ce_threshold,
		                     obj->fq_codel.memory,
-		                     obj->fq_codel.ecn == TRUE);
+		                     NM_HASH_COMBINE_BOOLS (guint8,
+		                                            obj->fq_codel.ecn));
	}
 }

@@ -6503,7 +6504,7 @@ nm_platform_qdisc_cmp (const NMPlatformQdisc *a, const NMPlatformQdisc *b)
		NM_CMP_FIELD (a, b, fq_codel.quantum);
		NM_CMP_FIELD (a, b, fq_codel.ce_threshold);
		NM_CMP_FIELD (a, b, fq_codel.memory);
-		NM_CMP_FIELD (a, b, fq_codel.ecn == TRUE);
+		NM_CMP_FIELD_UNSAFE (a, b, fq_codel.ecn);
	}

	return 0;
--
2.21.0

From ef2b660bb8cb2c30faf55c51a411e8a757707076 Mon Sep 17 00:00:00 2001
From: Thomas Haller <thaller@redhat.com>
Date: Wed, 1 May 2019 08:20:42 +0200
Subject: [PATCH 10/20] platform: use u32 netlink type for TCA_FQ_CODEL_ECN

In practice, there is no difference when representing 0 or 1 as signed/unsigned 32
bit integer. But still use the correct type that also kernel uses.

Also, the implicit conversation from uint32 to bool was correct already.
Still, explicitly convert the uint32 value to boolean in _new_from_nl_qdisc().
It's no change in behavior.

(cherry picked from commit a1099a1fab661a430fa8e8015369b536c806433e)
---
 src/platform/nm-linux-platform.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/platform/nm-linux-platform.c b/src/platform/nm-linux-platform.c
index 6064d89eb6..1428778e03 100644
--- a/src/platform/nm-linux-platform.c
+++ b/src/platform/nm-linux-platform.c
@@ -3547,7 +3547,7 @@ _new_from_nl_qdisc (struct nlmsghdr *nlh, gboolean id_only)
					obj->qdisc.fq_codel.memory = nla_get_u32 (options_attr);
					break;
				case TCA_FQ_CODEL_ECN:
-					obj->qdisc.fq_codel.ecn = nla_get_u32 (options_attr);
+					obj->qdisc.fq_codel.ecn = !!nla_get_u32 (options_attr);
					break;
				}
			}
@@ -4244,7 +4244,7 @@ _nl_msg_new_qdisc (int nlmsg_type,
		if (qdisc->fq_codel.memory != -1)
			NLA_PUT_U32 (msg, TCA_FQ_CODEL_MEMORY_LIMIT, qdisc->fq_codel.memory);
		if (qdisc->fq_codel.ecn)
-			NLA_PUT_S32 (msg, TCA_FQ_CODEL_ECN, qdisc->fq_codel.ecn);
+			NLA_PUT_U32 (msg, TCA_FQ_CODEL_ECN, qdisc->fq_codel.ecn);
	}

	nla_nest_end (msg, tc_options);
--
2.21.0

From dd3ca10284f9f7f95b1f313a3aa678f2aef01b3f Mon Sep 17 00:00:00 2001
From: Thomas Haller <thaller@redhat.com>
Date: Wed, 1 May 2019 08:23:00 +0200
Subject: [PATCH 11/20] platform: fix nm_platform_qdisc_to_string()

When using nm_utils_strbuf_*() API, the buffer gets always moved to the current
end. We must thus remember and return the original start of the buffer.

(cherry picked from commit b658e3da0825cf6e62e0850d3508dde1dd5c1914)
---
 src/platform/nm-platform.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/platform/nm-platform.c b/src/platform/nm-platform.c
index f8cf0b8999..4d3b61405d 100644
--- a/src/platform/nm-platform.c
+++ b/src/platform/nm-platform.c
@@ -6427,10 +6427,13 @@ const char *
 nm_platform_qdisc_to_string (const NMPlatformQdisc *qdisc, char *buf, gsize len)
 {
	char str_dev[TO_STRING_DEV_BUF_SIZE];
+	const char *buf0;

	if (!nm_utils_to_string_buffer_init_null (qdisc, &buf, &len))
		return buf;

+	buf0 = buf;
+
	nm_utils_strbuf_append (&buf, &len, "%s%s family %u handle %x parent %x info %x",
	                        qdisc->kind,
	                        _to_string_dev (NULL, qdisc->ifindex, str_dev, sizeof (str_dev)),
@@ -6458,7 +6461,7 @@ nm_platform_qdisc_to_string (const NMPlatformQdisc *qdisc, char *buf, gsize len)
			nm_utils_strbuf_append (&buf, &len, " ecn");
	}

-	return buf;
+	return buf0;
 }

 void
--
2.21.0

From 509a1bc5f2e52f6d639b75a0467b584c08a90463 Mon Sep 17 00:00:00 2001
From: Thomas Haller <thaller@redhat.com>
Date: Wed, 1 May 2019 08:43:31 +0200
Subject: [PATCH 12/20] platform: fix handling of fq_codel's memory limit
 default value

The memory-limit is an unsigned integer. It is ugly (if not wrong) to compare unsigned
values with "-1". When comparing with the default value we must also use an u32 type.
Instead add a define NM_PLATFORM_FQ_CODEL_MEMORY_LIMIT_UNSET.

Note that like iproute2 we treat NM_PLATFORM_FQ_CODEL_MEMORY_LIMIT_UNSET
to indicate to not set TCA_FQ_CODEL_MEMORY_LIMIT in RTM_NEWQDISC. This
special value is entirely internal to NetworkManager (or iproute2) and
kernel will then choose a default memory limit (of 32MB). So setting
NM_PLATFORM_FQ_CODEL_MEMORY_LIMIT_UNSET means to leave it to kernel to
choose a value (which then chooses 32MB).

See kernel's net/sched/sch_fq_codel.c:

    static int fq_codel_init(struct Qdisc *sch, struct nlattr *opt,
                             struct netlink_ext_ack *extack)
    {
    ...
            q->memory_limit = 32 << 20; /* 32 MBytes */

    static int fq_codel_change(struct Qdisc *sch, struct nlattr *opt,
                               struct netlink_ext_ack *extack)
    ...
            if (tb[TCA_FQ_CODEL_MEMORY_LIMIT])
                    q->memory_limit = min(1U << 31, nla_get_u32(tb[TCA_FQ_CODEL_MEMORY_LIMIT]));

Note that not having zero as default value is problematic. In fields like
"NMPlatformIP4Route.table_coerced" and "NMPlatformRoutingRule.suppress_prefixlen_inverse"
we avoid this problem by storing a coerced value in the structure so that zero is still
the default. We don't do that here for memory-limit, so the caller must always explicitly
set the value.

(cherry picked from commit 46a904389bf13a2cfd40888ab2a843827fda7469)
---
 libnm-core/nm-utils.c            | 5 +++++
 src/devices/nm-device.c          | 2 +-
 src/platform/nm-linux-platform.c | 5 ++++-
 src/platform/nm-platform.c       | 2 +-
 src/platform/nm-platform.h       | 9 ++++++++-
 5 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/libnm-core/nm-utils.c b/libnm-core/nm-utils.c
index 7cf482d3de..2e66e6ae2d 100644
--- a/libnm-core/nm-utils.c
+++ b/libnm-core/nm-utils.c
@@ -2334,7 +2334,12 @@ static const NMVariantAttributeSpec *const tc_qdisc_fq_codel_spec[] = {
	NM_VARIANT_ATTRIBUTE_SPEC_DEFINE ("interval",     G_VARIANT_TYPE_UINT32,                    ),
	NM_VARIANT_ATTRIBUTE_SPEC_DEFINE ("quantum",      G_VARIANT_TYPE_UINT32,                    ),
	NM_VARIANT_ATTRIBUTE_SPEC_DEFINE ("ce_threshold", G_VARIANT_TYPE_UINT32,                    ),
+
+	/* kernel clamps the value at 2^31. Possibly such values should be rejected from configuration
+	 * as they cannot be configured. Leaving the attribute unspecified causes kernel to choose
+	 * a default (currently 32MB). */
	NM_VARIANT_ATTRIBUTE_SPEC_DEFINE ("memory",       G_VARIANT_TYPE_UINT32,                    ),
+
	NM_VARIANT_ATTRIBUTE_SPEC_DEFINE ("ecn",          G_VARIANT_TYPE_BOOLEAN, .no_value = TRUE, ),
	NULL,
 };
diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c
index e07798094f..ce50d5b73d 100644
--- a/src/devices/nm-device.c
+++ b/src/devices/nm-device.c
@@ -6539,7 +6539,7 @@ tc_commit (NMDevice *self)
				GET_ATTR("interval", qdisc->fq_codel.interval, UINT32, uint32, 0);
				GET_ATTR("quantum", qdisc->fq_codel.quantum, UINT32, uint32, 0);
				GET_ATTR("ce_threshold", qdisc->fq_codel.ce_threshold, UINT32, uint32, -1);
-				GET_ATTR("memory", qdisc->fq_codel.memory, UINT32, uint32, -1);
+				GET_ATTR("memory", qdisc->fq_codel.memory, UINT32, uint32, NM_PLATFORM_FQ_CODEL_MEMORY_LIMIT_UNSET);
				GET_ATTR("ecn", qdisc->fq_codel.ecn, BOOLEAN, boolean, FALSE);
			}

diff --git a/src/platform/nm-linux-platform.c b/src/platform/nm-linux-platform.c
index 1428778e03..cec5fb9431 100644
--- a/src/platform/nm-linux-platform.c
+++ b/src/platform/nm-linux-platform.c
@@ -3515,6 +3515,9 @@ _new_from_nl_qdisc (struct nlmsghdr *nlh, gboolean id_only)
	obj->qdisc.parent = tcm->tcm_parent;
	obj->qdisc.info = tcm->tcm_info;

+	if (nm_streq0 (obj->qdisc.kind, "fq_codel"))
+		obj->qdisc.fq_codel.memory = NM_PLATFORM_FQ_CODEL_MEMORY_LIMIT_UNSET;
+
	if (tb[TCA_OPTIONS]) {
		struct nlattr *options_attr;
		int remaining;
@@ -4241,7 +4244,7 @@ _nl_msg_new_qdisc (int nlmsg_type,
			NLA_PUT_U32 (msg, TCA_FQ_CODEL_QUANTUM, qdisc->fq_codel.quantum);
		if (qdisc->fq_codel.ce_threshold != -1)
			NLA_PUT_U32 (msg, TCA_FQ_CODEL_CE_THRESHOLD, qdisc->fq_codel.ce_threshold);
-		if (qdisc->fq_codel.memory != -1)
+		if (qdisc->fq_codel.memory != NM_PLATFORM_FQ_CODEL_MEMORY_LIMIT_UNSET)
			NLA_PUT_U32 (msg, TCA_FQ_CODEL_MEMORY_LIMIT, qdisc->fq_codel.memory);
		if (qdisc->fq_codel.ecn)
			NLA_PUT_U32 (msg, TCA_FQ_CODEL_ECN, qdisc->fq_codel.ecn);
diff --git a/src/platform/nm-platform.c b/src/platform/nm-platform.c
index 4d3b61405d..fc49db19a1 100644
--- a/src/platform/nm-platform.c
+++ b/src/platform/nm-platform.c
@@ -6455,7 +6455,7 @@ nm_platform_qdisc_to_string (const NMPlatformQdisc *qdisc, char *buf, gsize len)
			nm_utils_strbuf_append (&buf, &len, " quantum %u", qdisc->fq_codel.quantum);
		if (qdisc->fq_codel.ce_threshold != -1)
			nm_utils_strbuf_append (&buf, &len, " ce_threshold %u", qdisc->fq_codel.ce_threshold);
-		if (qdisc->fq_codel.memory != -1)
+		if (qdisc->fq_codel.memory != NM_PLATFORM_FQ_CODEL_MEMORY_LIMIT_UNSET)
			nm_utils_strbuf_append (&buf, &len, " memory %u", qdisc->fq_codel.memory);
		if (qdisc->fq_codel.ecn)
			nm_utils_strbuf_append (&buf, &len, " ecn");
diff --git a/src/platform/nm-platform.h b/src/platform/nm-platform.h
index 16747f093b..9f9fcf5c31 100644
--- a/src/platform/nm-platform.h
+++ b/src/platform/nm-platform.h
@@ -596,6 +596,8 @@ typedef struct {
	bool     uid_range_has:1;            /* has(FRA_UID_RANGE) */
 } NMPlatformRoutingRule;

+#define NM_PLATFORM_FQ_CODEL_MEMORY_LIMIT_UNSET   (~((guint32) 0))
+
 typedef struct {
	guint32 limit;
	guint32 flows;
@@ -603,7 +605,12 @@ typedef struct {
	guint32 interval;
	guint32 quantum;
	guint32 ce_threshold;
-	guint32 memory;
+	guint32 memory;       /* TCA_FQ_CODEL_MEMORY_LIMIT: note that only values <= 2^31 are accepted by kernel
+	                       *   and kernel defaults to 32MB.
+	                       *   Note that we use the special value NM_PLATFORM_FQ_CODEL_MEMORY_LIMIT_UNSET
+	                       *   to indicate that no explicit limit is set (when we send a RTM_NEWQDISC request).
+	                       *   This will cause kernel to choose the default (32MB).
+	                       *   Beware: zero is not the default you must always explicitly set this value. */
	bool ecn:1;
 } NMPlatformQdiscFqCodel;

--
2.21.0

From 859f8479d453c9270987245b9d6e537af0e42077 Mon Sep 17 00:00:00 2001
From: Thomas Haller <thaller@redhat.com>
Date: Wed, 1 May 2019 09:19:59 +0200
Subject: [PATCH 13/20] platform: fix handling of default value for
 TCA_FQ_CODEL_CE_THRESHOLD

iproute2 uses the special value ~0u to indicate not to set
TCA_FQ_CODEL_CE_THRESHOLD in RTM_NEWQDISC. When not explicitly
setting the value, kernel treats the threshold as disabled.

However note that 0xFFFFFFFFu is not an invalid threshold (as far as
kernel is concerned). Thus, we should not use that as value to indicate
that the value is unset. Note that iproute2 uses the special value ~0u
only internally thereby making it impossible to set the threshold to
0xFFFFFFFFu). But kernel does not have this limitation.

Maybe the cleanest way would be to add another field to NMPlatformQDisc:

    guint32 ce_threshold;
    bool ce_threshold_set:1;

that indicates whether the threshold is enable or not.
But note that kernel does:

    static void codel_params_init(struct codel_params *params)
    {
    ...
            params->ce_threshold = CODEL_DISABLED_THRESHOLD;

    static int fq_codel_change(struct Qdisc *sch, struct nlattr *opt,
                               struct netlink_ext_ack *extack)
    {
    ...
            if (tb[TCA_FQ_CODEL_CE_THRESHOLD]) {
                    u64 val = nla_get_u32(tb[TCA_FQ_CODEL_CE_THRESHOLD]);

                    q->cparams.ce_threshold = (val * NSEC_PER_USEC) >> CODEL_SHIFT;
            }

    static int fq_codel_dump(struct Qdisc *sch, struct sk_buff *skb)
    {
    ...
            if (q->cparams.ce_threshold != CODEL_DISABLED_THRESHOLD &&
                nla_put_u32(skb, TCA_FQ_CODEL_CE_THRESHOLD,
                            codel_time_to_us(q->cparams.ce_threshold)))
                    goto nla_put_failure;

This means, kernel internally uses the special value 0x83126E97u to indicate
that the threshold is disabled (WTF). That is because

  (((guint64) 0x83126E97u) * NSEC_PER_USEC) >> CODEL_SHIFT == CODEL_DISABLED_THRESHOLD

So in kernel API this value is reserved (and has a special meaning
to indicate that the threshold is disabled). So, instead of adding a
ce_threshold_set flag, use the same value that kernel anyway uses.

(cherry picked from commit 973db2d41b957c4ee9d4ee9863f4b35c6890ac30)
---
 libnm-core/nm-utils.c            |  3 +++
 src/devices/nm-device.c          |  2 +-
 src/platform/nm-linux-platform.c |  6 ++++--
 src/platform/nm-platform.c       |  2 +-
 src/platform/nm-platform.h       | 12 +++++++++++-
 5 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/libnm-core/nm-utils.c b/libnm-core/nm-utils.c
index 2e66e6ae2d..9e2eb63594 100644
--- a/libnm-core/nm-utils.c
+++ b/libnm-core/nm-utils.c
@@ -2333,6 +2333,9 @@ static const NMVariantAttributeSpec *const tc_qdisc_fq_codel_spec[] = {
	NM_VARIANT_ATTRIBUTE_SPEC_DEFINE ("target",       G_VARIANT_TYPE_UINT32,                    ),
	NM_VARIANT_ATTRIBUTE_SPEC_DEFINE ("interval",     G_VARIANT_TYPE_UINT32,                    ),
	NM_VARIANT_ATTRIBUTE_SPEC_DEFINE ("quantum",      G_VARIANT_TYPE_UINT32,                    ),
+
+	/* 0x83126E97u is not a valid value (it means "disabled"). We should reject that
+	 * value. Or alternatively, reject all values >= MAX_INT(32). */
	NM_VARIANT_ATTRIBUTE_SPEC_DEFINE ("ce_threshold", G_VARIANT_TYPE_UINT32,                    ),

	/* kernel clamps the value at 2^31. Possibly such values should be rejected from configuration
diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c
index ce50d5b73d..22ccabb26e 100644
--- a/src/devices/nm-device.c
+++ b/src/devices/nm-device.c
@@ -6538,7 +6538,7 @@ tc_commit (NMDevice *self)
				GET_ATTR("target", qdisc->fq_codel.target, UINT32, uint32, 0);
				GET_ATTR("interval", qdisc->fq_codel.interval, UINT32, uint32, 0);
				GET_ATTR("quantum", qdisc->fq_codel.quantum, UINT32, uint32, 0);
-				GET_ATTR("ce_threshold", qdisc->fq_codel.ce_threshold, UINT32, uint32, -1);
+				GET_ATTR("ce_threshold", qdisc->fq_codel.ce_threshold, UINT32, uint32, NM_PLATFORM_FQ_CODEL_CE_THRESHOLD_DISABLED);
				GET_ATTR("memory", qdisc->fq_codel.memory, UINT32, uint32, NM_PLATFORM_FQ_CODEL_MEMORY_LIMIT_UNSET);
				GET_ATTR("ecn", qdisc->fq_codel.ecn, BOOLEAN, boolean, FALSE);
			}
diff --git a/src/platform/nm-linux-platform.c b/src/platform/nm-linux-platform.c
index cec5fb9431..2e11052e1a 100644
--- a/src/platform/nm-linux-platform.c
+++ b/src/platform/nm-linux-platform.c
@@ -3515,8 +3515,10 @@ _new_from_nl_qdisc (struct nlmsghdr *nlh, gboolean id_only)
	obj->qdisc.parent = tcm->tcm_parent;
	obj->qdisc.info = tcm->tcm_info;

-	if (nm_streq0 (obj->qdisc.kind, "fq_codel"))
+	if (nm_streq0 (obj->qdisc.kind, "fq_codel")) {
		obj->qdisc.fq_codel.memory = NM_PLATFORM_FQ_CODEL_MEMORY_LIMIT_UNSET;
+		obj->qdisc.fq_codel.ce_threshold = NM_PLATFORM_FQ_CODEL_CE_THRESHOLD_DISABLED;
+	}

	if (tb[TCA_OPTIONS]) {
		struct nlattr *options_attr;
@@ -4242,7 +4244,7 @@ _nl_msg_new_qdisc (int nlmsg_type,
			NLA_PUT_U32 (msg, TCA_FQ_CODEL_INTERVAL, qdisc->fq_codel.interval);
		if (qdisc->fq_codel.quantum)
			NLA_PUT_U32 (msg, TCA_FQ_CODEL_QUANTUM, qdisc->fq_codel.quantum);
-		if (qdisc->fq_codel.ce_threshold != -1)
+		if (qdisc->fq_codel.ce_threshold != NM_PLATFORM_FQ_CODEL_CE_THRESHOLD_DISABLED)
			NLA_PUT_U32 (msg, TCA_FQ_CODEL_CE_THRESHOLD, qdisc->fq_codel.ce_threshold);
		if (qdisc->fq_codel.memory != NM_PLATFORM_FQ_CODEL_MEMORY_LIMIT_UNSET)
			NLA_PUT_U32 (msg, TCA_FQ_CODEL_MEMORY_LIMIT, qdisc->fq_codel.memory);
diff --git a/src/platform/nm-platform.c b/src/platform/nm-platform.c
index fc49db19a1..cbf9eed5c1 100644
--- a/src/platform/nm-platform.c
+++ b/src/platform/nm-platform.c
@@ -6453,7 +6453,7 @@ nm_platform_qdisc_to_string (const NMPlatformQdisc *qdisc, char *buf, gsize len)
			nm_utils_strbuf_append (&buf, &len, " interval %u", qdisc->fq_codel.interval);
		if (qdisc->fq_codel.quantum)
			nm_utils_strbuf_append (&buf, &len, " quantum %u", qdisc->fq_codel.quantum);
-		if (qdisc->fq_codel.ce_threshold != -1)
+		if (qdisc->fq_codel.ce_threshold != NM_PLATFORM_FQ_CODEL_CE_THRESHOLD_DISABLED)
			nm_utils_strbuf_append (&buf, &len, " ce_threshold %u", qdisc->fq_codel.ce_threshold);
		if (qdisc->fq_codel.memory != NM_PLATFORM_FQ_CODEL_MEMORY_LIMIT_UNSET)
			nm_utils_strbuf_append (&buf, &len, " memory %u", qdisc->fq_codel.memory);
diff --git a/src/platform/nm-platform.h b/src/platform/nm-platform.h
index 9f9fcf5c31..d7c388b1b8 100644
--- a/src/platform/nm-platform.h
+++ b/src/platform/nm-platform.h
@@ -598,13 +598,23 @@ typedef struct {

 #define NM_PLATFORM_FQ_CODEL_MEMORY_LIMIT_UNSET   (~((guint32) 0))

+#define NM_PLATFORM_FQ_CODEL_CE_THRESHOLD_DISABLED ((guint32) 0x83126E97u)
+
+G_STATIC_ASSERT (((((guint64) NM_PLATFORM_FQ_CODEL_CE_THRESHOLD_DISABLED) * 1000u) >> 10) == (guint64) INT_MAX);
+
 typedef struct {
	guint32 limit;
	guint32 flows;
	guint32 target;
	guint32 interval;
	guint32 quantum;
-	guint32 ce_threshold;
+	guint32 ce_threshold; /* TCA_FQ_CODEL_CE_THRESHOLD: kernel internally stores this value as
+	                       *   ((val64 * NSEC_PER_USEC) >> CODEL_SHIFT). The default value (in
+	                       *   the domain with this coersion) is CODEL_DISABLED_THRESHOLD (INT_MAX).
+	                       *   That means, "disabled" is expressed on RTM_NEWQDISC netlink API by absence of the
+	                       *   netlink attribute but also as the special value 0x83126E97u
+	                       *   (NM_PLATFORM_FQ_CODEL_CE_THRESHOLD_DISABLED).
+	                       *   Beware: zero is not the default you must always explicitly set this value. */
	guint32 memory;       /* TCA_FQ_CODEL_MEMORY_LIMIT: note that only values <= 2^31 are accepted by kernel
	                       *   and kernel defaults to 32MB.
	                       *   Note that we use the special value NM_PLATFORM_FQ_CODEL_MEMORY_LIMIT_UNSET
--
2.21.0

From c17fa82b314d6b3c027240ee3506706e26ef9e5d Mon Sep 17 00:00:00 2001
From: Thomas Haller <thaller@redhat.com>
Date: Wed, 1 May 2019 12:56:46 +0200
Subject: [PATCH 14/20] libnm: rename "memory" parameter of fq_codel QDisc to
 "memory_limit"

Kernel calls the netlink attribute TCA_FQ_CODEL_MEMORY_LIMIT. Likewise,
iproute2 calls this "memory_limit".

Rename because TC parameters are inherrently tied to the kernel
implementation and we should use the familiar name.

(cherry picked from commit 666d58802b6f9072825d97498ab74e5d37652e93)
---
 libnm-core/nm-utils.c            | 2 +-
 src/devices/nm-device.c          | 2 +-
 src/platform/nm-linux-platform.c | 8 ++++----
 src/platform/nm-platform.c       | 8 ++++----
 src/platform/nm-platform.h       | 2 +-
 5 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/libnm-core/nm-utils.c b/libnm-core/nm-utils.c
index 9e2eb63594..394ca7c3a7 100644
--- a/libnm-core/nm-utils.c
+++ b/libnm-core/nm-utils.c
@@ -2341,7 +2341,7 @@ static const NMVariantAttributeSpec *const tc_qdisc_fq_codel_spec[] = {
	/* kernel clamps the value at 2^31. Possibly such values should be rejected from configuration
	 * as they cannot be configured. Leaving the attribute unspecified causes kernel to choose
	 * a default (currently 32MB). */
-	NM_VARIANT_ATTRIBUTE_SPEC_DEFINE ("memory",       G_VARIANT_TYPE_UINT32,                    ),
+	NM_VARIANT_ATTRIBUTE_SPEC_DEFINE ("memory_limit", G_VARIANT_TYPE_UINT32,                    ),

	NM_VARIANT_ATTRIBUTE_SPEC_DEFINE ("ecn",          G_VARIANT_TYPE_BOOLEAN, .no_value = TRUE, ),
	NULL,
diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c
index 22ccabb26e..e2a5fd4bf2 100644
--- a/src/devices/nm-device.c
+++ b/src/devices/nm-device.c
@@ -6539,7 +6539,7 @@ tc_commit (NMDevice *self)
				GET_ATTR("interval", qdisc->fq_codel.interval, UINT32, uint32, 0);
				GET_ATTR("quantum", qdisc->fq_codel.quantum, UINT32, uint32, 0);
				GET_ATTR("ce_threshold", qdisc->fq_codel.ce_threshold, UINT32, uint32, NM_PLATFORM_FQ_CODEL_CE_THRESHOLD_DISABLED);
-				GET_ATTR("memory", qdisc->fq_codel.memory, UINT32, uint32, NM_PLATFORM_FQ_CODEL_MEMORY_LIMIT_UNSET);
+				GET_ATTR("memory_limit", qdisc->fq_codel.memory_limit, UINT32, uint32, NM_PLATFORM_FQ_CODEL_MEMORY_LIMIT_UNSET);
				GET_ATTR("ecn", qdisc->fq_codel.ecn, BOOLEAN, boolean, FALSE);
			}

diff --git a/src/platform/nm-linux-platform.c b/src/platform/nm-linux-platform.c
index 2e11052e1a..9721ff9403 100644
--- a/src/platform/nm-linux-platform.c
+++ b/src/platform/nm-linux-platform.c
@@ -3516,7 +3516,7 @@ _new_from_nl_qdisc (struct nlmsghdr *nlh, gboolean id_only)
	obj->qdisc.info = tcm->tcm_info;

	if (nm_streq0 (obj->qdisc.kind, "fq_codel")) {
-		obj->qdisc.fq_codel.memory = NM_PLATFORM_FQ_CODEL_MEMORY_LIMIT_UNSET;
+		obj->qdisc.fq_codel.memory_limit = NM_PLATFORM_FQ_CODEL_MEMORY_LIMIT_UNSET;
		obj->qdisc.fq_codel.ce_threshold = NM_PLATFORM_FQ_CODEL_CE_THRESHOLD_DISABLED;
	}

@@ -3549,7 +3549,7 @@ _new_from_nl_qdisc (struct nlmsghdr *nlh, gboolean id_only)
					obj->qdisc.fq_codel.ce_threshold = nla_get_u32 (options_attr);
					break;
				case TCA_FQ_CODEL_MEMORY_LIMIT:
-					obj->qdisc.fq_codel.memory = nla_get_u32 (options_attr);
+					obj->qdisc.fq_codel.memory_limit = nla_get_u32 (options_attr);
					break;
				case TCA_FQ_CODEL_ECN:
					obj->qdisc.fq_codel.ecn = !!nla_get_u32 (options_attr);
@@ -4246,8 +4246,8 @@ _nl_msg_new_qdisc (int nlmsg_type,
			NLA_PUT_U32 (msg, TCA_FQ_CODEL_QUANTUM, qdisc->fq_codel.quantum);
		if (qdisc->fq_codel.ce_threshold != NM_PLATFORM_FQ_CODEL_CE_THRESHOLD_DISABLED)
			NLA_PUT_U32 (msg, TCA_FQ_CODEL_CE_THRESHOLD, qdisc->fq_codel.ce_threshold);
-		if (qdisc->fq_codel.memory != NM_PLATFORM_FQ_CODEL_MEMORY_LIMIT_UNSET)
-			NLA_PUT_U32 (msg, TCA_FQ_CODEL_MEMORY_LIMIT, qdisc->fq_codel.memory);
+		if (qdisc->fq_codel.memory_limit != NM_PLATFORM_FQ_CODEL_MEMORY_LIMIT_UNSET)
+			NLA_PUT_U32 (msg, TCA_FQ_CODEL_MEMORY_LIMIT, qdisc->fq_codel.memory_limit);
		if (qdisc->fq_codel.ecn)
			NLA_PUT_U32 (msg, TCA_FQ_CODEL_ECN, qdisc->fq_codel.ecn);
	}
diff --git a/src/platform/nm-platform.c b/src/platform/nm-platform.c
index cbf9eed5c1..ea73f21019 100644
--- a/src/platform/nm-platform.c
+++ b/src/platform/nm-platform.c
@@ -6455,8 +6455,8 @@ nm_platform_qdisc_to_string (const NMPlatformQdisc *qdisc, char *buf, gsize len)
			nm_utils_strbuf_append (&buf, &len, " quantum %u", qdisc->fq_codel.quantum);
		if (qdisc->fq_codel.ce_threshold != NM_PLATFORM_FQ_CODEL_CE_THRESHOLD_DISABLED)
			nm_utils_strbuf_append (&buf, &len, " ce_threshold %u", qdisc->fq_codel.ce_threshold);
-		if (qdisc->fq_codel.memory != NM_PLATFORM_FQ_CODEL_MEMORY_LIMIT_UNSET)
-			nm_utils_strbuf_append (&buf, &len, " memory %u", qdisc->fq_codel.memory);
+		if (qdisc->fq_codel.memory_limit != NM_PLATFORM_FQ_CODEL_MEMORY_LIMIT_UNSET)
+			nm_utils_strbuf_append (&buf, &len, " memory_limit %u", qdisc->fq_codel.memory_limit);
		if (qdisc->fq_codel.ecn)
			nm_utils_strbuf_append (&buf, &len, " ecn");
	}
@@ -6482,7 +6482,7 @@ nm_platform_qdisc_hash_update (const NMPlatformQdisc *obj, NMHashState *h)
		                     obj->fq_codel.interval,
		                     obj->fq_codel.quantum,
		                     obj->fq_codel.ce_threshold,
-		                     obj->fq_codel.memory,
+		                     obj->fq_codel.memory_limit,
		                     NM_HASH_COMBINE_BOOLS (guint8,
		                                            obj->fq_codel.ecn));
	}
@@ -6506,7 +6506,7 @@ nm_platform_qdisc_cmp (const NMPlatformQdisc *a, const NMPlatformQdisc *b)
		NM_CMP_FIELD (a, b, fq_codel.interval);
		NM_CMP_FIELD (a, b, fq_codel.quantum);
		NM_CMP_FIELD (a, b, fq_codel.ce_threshold);
-		NM_CMP_FIELD (a, b, fq_codel.memory);
+		NM_CMP_FIELD (a, b, fq_codel.memory_limit);
		NM_CMP_FIELD_UNSAFE (a, b, fq_codel.ecn);
	}

diff --git a/src/platform/nm-platform.h b/src/platform/nm-platform.h
index d7c388b1b8..5a0efceca7 100644
--- a/src/platform/nm-platform.h
+++ b/src/platform/nm-platform.h
@@ -615,7 +615,7 @@ typedef struct {
	                       *   netlink attribute but also as the special value 0x83126E97u
	                       *   (NM_PLATFORM_FQ_CODEL_CE_THRESHOLD_DISABLED).
	                       *   Beware: zero is not the default you must always explicitly set this value. */
-	guint32 memory;       /* TCA_FQ_CODEL_MEMORY_LIMIT: note that only values <= 2^31 are accepted by kernel
+	guint32 memory_limit; /* TCA_FQ_CODEL_MEMORY_LIMIT: note that only values <= 2^31 are accepted by kernel
	                       *   and kernel defaults to 32MB.
	                       *   Note that we use the special value NM_PLATFORM_FQ_CODEL_MEMORY_LIMIT_UNSET
	                       *   to indicate that no explicit limit is set (when we send a RTM_NEWQDISC request).
--
2.21.0

From 79e3b2a838b1ae7e6174de8ff000509e918d5c90 Mon Sep 17 00:00:00 2001
From: Thomas Haller <thaller@redhat.com>
Date: Wed, 1 May 2019 12:58:37 +0200
Subject: [PATCH 15/20] platform: use bool bitfields in NMPlatformActionMirred
 structure

Arguably, the structure is used inside a union with another (larger)
struct, hence no memory is saved.

In fact, it may well be slower performance wise to access a boolean bitfield
than a gboolean (int).

Still, boolean fields in structures should be bool:1 bitfields for
consistency.

(cherry picked from commit 36d6aa3bcd97f2144e8f435249ed8f3cb709ae43)
---
 src/platform/nm-platform.c | 19 ++++++++++---------
 src/platform/nm-platform.h |  8 ++++----
 2 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/src/platform/nm-platform.c b/src/platform/nm-platform.c
index ea73f21019..ee71319dd6 100644
--- a/src/platform/nm-platform.c
+++ b/src/platform/nm-platform.c
@@ -6577,11 +6577,12 @@ nm_platform_tfilter_hash_update (const NMPlatformTfilter *obj, NMHashState *h)
			nm_hash_update_strarr (h, obj->action.simple.sdata);
		} else if (nm_streq (obj->action.kind, NM_PLATFORM_ACTION_KIND_MIRRED)) {
			nm_hash_update_vals (h,
-			                     obj->action.mirred.ingress,
-			                     obj->action.mirred.egress,
-			                     obj->action.mirred.mirror,
-			                     obj->action.mirred.redirect,
-			                     obj->action.mirred.ifindex);
+			                     obj->action.mirred.ifindex,
+			                     NM_HASH_COMBINE_BOOLS (guint8,
+			                                            obj->action.mirred.ingress,
+			                                            obj->action.mirred.egress,
+			                                            obj->action.mirred.mirror,
+			                                            obj->action.mirred.redirect));
		}
	}
 }
@@ -6602,11 +6603,11 @@ nm_platform_tfilter_cmp (const NMPlatformTfilter *a, const NMPlatformTfilter *b)
		if (nm_streq (a->action.kind, NM_PLATFORM_ACTION_KIND_SIMPLE)) {
			NM_CMP_FIELD_STR (a, b, action.simple.sdata);
		} else if (nm_streq (a->action.kind, NM_PLATFORM_ACTION_KIND_MIRRED)) {
-			NM_CMP_FIELD (a, b, action.mirred.ingress);
-			NM_CMP_FIELD (a, b, action.mirred.egress);
-			NM_CMP_FIELD (a, b, action.mirred.mirror);
-			NM_CMP_FIELD (a, b, action.mirred.redirect);
			NM_CMP_FIELD (a, b, action.mirred.ifindex);
+			NM_CMP_FIELD_UNSAFE (a, b, action.mirred.ingress);
+			NM_CMP_FIELD_UNSAFE (a, b, action.mirred.egress);
+			NM_CMP_FIELD_UNSAFE (a, b, action.mirred.mirror);
+			NM_CMP_FIELD_UNSAFE (a, b, action.mirred.redirect);
		}
	}

diff --git a/src/platform/nm-platform.h b/src/platform/nm-platform.h
index 5a0efceca7..9b6848d977 100644
--- a/src/platform/nm-platform.h
+++ b/src/platform/nm-platform.h
@@ -641,11 +641,11 @@ typedef struct {
 } NMPlatformActionSimple;

 typedef struct {
-	gboolean egress;
-	gboolean ingress;
-	gboolean mirror;
-	gboolean redirect;
	int ifindex;
+	bool egress:1;
+	bool ingress:1;
+	bool mirror:1;
+	bool redirect:1;
 } NMPlatformActionMirred;

 typedef struct {
--
2.21.0

From 8b1b398c0545f31121c52d323276e013705ddf80 Mon Sep 17 00:00:00 2001
From: Thomas Haller <thaller@redhat.com>
Date: Wed, 1 May 2019 13:07:43 +0200
Subject: [PATCH 16/20] platform: assert for out-of-memory in netlink code

These lines can be reached if the allocated buffer is too
small to hold the netlink message. That is actually a bug
that we need to fix. Assert.

(cherry picked from commit 3784a2a2ec6f8c6307f45bae12c17630b7d70b0a)
---
 src/platform/nm-linux-platform.c          | 10 +++++-----
 src/platform/wifi/nm-wifi-utils-nl80211.c | 12 ++++++------
 src/platform/wpan/nm-wpan-utils.c         |  8 ++++----
 3 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/src/platform/nm-linux-platform.c b/src/platform/nm-linux-platform.c
index 9721ff9403..6dd5b4ab3e 100644
--- a/src/platform/nm-linux-platform.c
+++ b/src/platform/nm-linux-platform.c
@@ -3681,7 +3681,7 @@ _nl_msg_new_link_set_afspec (struct nl_msg *msg,

	return TRUE;
 nla_put_failure:
-	return FALSE;
+	g_return_val_if_reached (FALSE);
 }

 static gboolean
@@ -3832,7 +3832,7 @@ _nl_msg_new_link_set_linkinfo_vlan (struct nl_msg *msg,

	return TRUE;
 nla_put_failure:
-	return FALSE;
+	g_return_val_if_reached (FALSE);
 }

 static struct nl_msg *
@@ -4278,7 +4278,7 @@ _add_action_simple (struct nl_msg *msg,
	return TRUE;

 nla_put_failure:
-	return FALSE;
+	g_return_val_if_reached (FALSE);
 }

 static gboolean
@@ -4308,7 +4308,7 @@ _add_action_mirred (struct nl_msg *msg,
	return TRUE;

 nla_put_failure:
-	return FALSE;
+	g_return_val_if_reached (FALSE);
 }

 static gboolean
@@ -4334,7 +4334,7 @@ _add_action (struct nl_msg *msg,
	return TRUE;

 nla_put_failure:
-	return FALSE;
+	g_return_val_if_reached (FALSE);
 }

 static struct nl_msg *
diff --git a/src/platform/wifi/nm-wifi-utils-nl80211.c b/src/platform/wifi/nm-wifi-utils-nl80211.c
index 4f7ede9757..6a8525ed4b 100644
--- a/src/platform/wifi/nm-wifi-utils-nl80211.c
+++ b/src/platform/wifi/nm-wifi-utils-nl80211.c
@@ -103,7 +103,7 @@ _nl80211_alloc_msg (int id, int ifindex, int phy, guint32 cmd, guint32 flags)
	return g_steal_pointer (&msg);

 nla_put_failure:
-	return NULL;
+	g_return_val_if_reached (NULL);
 }

 static struct nl_msg *
@@ -250,7 +250,7 @@ wifi_nl80211_set_mode (NMWifiUtils *data, const NM80211Mode mode)
	return err >= 0;

 nla_put_failure:
-	return FALSE;
+	g_return_val_if_reached (FALSE);
 }

 static gboolean
@@ -267,7 +267,7 @@ wifi_nl80211_set_powersave (NMWifiUtils *data, guint32 powersave)
	return err >= 0;

 nla_put_failure:
-	return FALSE;
+	g_return_val_if_reached (FALSE);
 }

 static int
@@ -365,7 +365,7 @@ wifi_nl80211_set_wake_on_wlan (NMWifiUtils *data, NMSettingWirelessWakeOnWLan wo
	return err >= 0;

 nla_put_failure:
-	return FALSE;
+	g_return_val_if_reached (FALSE);
 }

 /* @divisor: pass what value @xbm should be divided by to get dBm */
@@ -642,7 +642,7 @@ nl80211_get_ap_info (NMWifiUtilsNl80211 *self,
	return;

 nla_put_failure:
-	return;
+	g_return_if_reached ();
 }

 static guint32
@@ -695,7 +695,7 @@ wifi_nl80211_indicate_addressing_running (NMWifiUtils *data, gboolean running)
	return err >= 0;

 nla_put_failure:
-	return FALSE;
+	g_return_val_if_reached (FALSE);
 }

 struct nl80211_device_info {
diff --git a/src/platform/wpan/nm-wpan-utils.c b/src/platform/wpan/nm-wpan-utils.c
index b7a51e9bf2..0afc2a4d0c 100644
--- a/src/platform/wpan/nm-wpan-utils.c
+++ b/src/platform/wpan/nm-wpan-utils.c
@@ -92,7 +92,7 @@ _nl802154_alloc_msg (int id, int ifindex, guint32 cmd, guint32 flags)
	return g_steal_pointer (&msg);

 nla_put_failure:
-	return NULL;
+	g_return_val_if_reached (NULL);
 }

 static struct nl_msg *
@@ -217,7 +217,7 @@ nm_wpan_utils_set_pan_id (NMWpanUtils *self, guint16 pan_id)
	return err >= 0;

 nla_put_failure:
-	return FALSE;
+	g_return_val_if_reached (FALSE);
 }

 guint16
@@ -244,7 +244,7 @@ nm_wpan_utils_set_short_addr (NMWpanUtils *self, guint16 short_addr)
	return err >= 0;

 nla_put_failure:
-	return FALSE;
+	g_return_val_if_reached (FALSE);
 }

 gboolean
@@ -262,7 +262,7 @@ nm_wpan_utils_set_channel (NMWpanUtils *self, guint8 page, guint8 channel)
	return err >= 0;

 nla_put_failure:
-	return FALSE;
+	g_return_val_if_reached (FALSE);
 }

 /*****************************************************************************/
--
2.21.0

From 27341d042487afabcd48fbc08054069f89b9c424 Mon Sep 17 00:00:00 2001
From: Thomas Haller <thaller@redhat.com>
Date: Wed, 1 May 2019 13:15:57 +0200
Subject: [PATCH 17/20] platform: merge _add_action(), _add_action_simple() and
 _add_action_mirred() into _nl_msg_new_tfilter()

There is only one caller, hence it's simpler to see it all in one place.
I prefer this, because then I can read the code top to bottom and
see what's happening, without following helper functions.

Also, this way we can "reuse" the nla_put_failure label and assertion. Previously,
if the assertion was hit we would not rewind the buffer but continue
constructing the message (which is already borked). Not that it matters
too much, because this was on an "failed-assertion" code path.

(cherry picked from commit 04bd404dffc8bc1ec5c3fed91e75fad1a1b1a4d3)
---
 src/platform/nm-linux-platform.c | 125 ++++++++++++-------------------
 1 file changed, 46 insertions(+), 79 deletions(-)

diff --git a/src/platform/nm-linux-platform.c b/src/platform/nm-linux-platform.c
index 6dd5b4ab3e..4a62a76485 100644
--- a/src/platform/nm-linux-platform.c
+++ b/src/platform/nm-linux-platform.c
@@ -4260,83 +4260,6 @@ nla_put_failure:
	g_return_val_if_reached (NULL);
 }

-static gboolean
-_add_action_simple (struct nl_msg *msg,
-                    const NMPlatformActionSimple *simple)
-{
-	struct nlattr *act_options;
-	struct tc_defact sel = { 0, };
-
-	if (!(act_options = nla_nest_start (msg, TCA_ACT_OPTIONS)))
-		goto nla_put_failure;
-
-	NLA_PUT (msg, TCA_DEF_PARMS, sizeof (sel), &sel);
-	NLA_PUT (msg, TCA_DEF_DATA, sizeof (simple->sdata), simple->sdata);
-
-	nla_nest_end (msg, act_options);
-
-	return TRUE;
-
-nla_put_failure:
-	g_return_val_if_reached (FALSE);
-}
-
-static gboolean
-_add_action_mirred (struct nl_msg *msg,
-                    const NMPlatformActionMirred *mirred)
-{
-	struct nlattr *act_options;
-	struct tc_mirred sel = { 0, };
-
-	if (!(act_options = nla_nest_start (msg, TCA_ACT_OPTIONS)))
-		goto nla_put_failure;
-
-	if (mirred->egress && mirred->redirect)
-		sel.eaction = TCA_EGRESS_REDIR;
-	else if (mirred->egress && mirred->mirror)
-		sel.eaction = TCA_EGRESS_MIRROR;
-	else if (mirred->ingress && mirred->redirect)
-		sel.eaction = TCA_INGRESS_REDIR;
-	else if (mirred->ingress && mirred->mirror)
-		sel.eaction = TCA_INGRESS_MIRROR;
-	sel.ifindex = mirred->ifindex;
-
-	NLA_PUT (msg, TCA_MIRRED_PARMS, sizeof (sel), &sel);
-
-	nla_nest_end (msg, act_options);
-
-	return TRUE;
-
-nla_put_failure:
-	g_return_val_if_reached (FALSE);
-}
-
-static gboolean
-_add_action (struct nl_msg *msg,
-             const NMPlatformAction *action)
-{
-	struct nlattr *prio;
-
-	nm_assert (action || action->kind);
-
-	if (!(prio = nla_nest_start (msg, 1 /* priority */)))
-		goto nla_put_failure;
-
-	NLA_PUT_STRING (msg, TCA_ACT_KIND, action->kind);
-
-	if (nm_streq (action->kind, NM_PLATFORM_ACTION_KIND_SIMPLE))
-		_add_action_simple (msg, &action->simple);
-	else if (nm_streq (action->kind, NM_PLATFORM_ACTION_KIND_MIRRED))
-		_add_action_mirred (msg, &action->mirred);
-
-	nla_nest_end (msg, prio);
-
-	return TRUE;
-
-nla_put_failure:
-	g_return_val_if_reached (FALSE);
-}
-
 static struct nl_msg *
 _nl_msg_new_tfilter (int nlmsg_type,
                      int nlmsg_flags,
@@ -4366,8 +4289,52 @@ _nl_msg_new_tfilter (int nlmsg_type,
	if (!(act_tab = nla_nest_start (msg, TCA_OPTIONS))) // 3 TCA_ACT_KIND TCA_ACT_KIND
		goto nla_put_failure;

-	if (tfilter->action.kind)
-		_add_action (msg, &tfilter->action);
+	if (tfilter->action.kind) {
+		const NMPlatformAction *action = &tfilter->action;
+		struct nlattr *prio;
+		struct nlattr *act_options;
+
+		if (!(prio = nla_nest_start (msg, 1 /* priority */)))
+			goto nla_put_failure;
+
+		NLA_PUT_STRING (msg, TCA_ACT_KIND, action->kind);
+
+		if (nm_streq (action->kind, NM_PLATFORM_ACTION_KIND_SIMPLE)) {
+			const NMPlatformActionSimple *simple = &action->simple;
+			struct tc_defact sel = { 0, };
+
+			if (!(act_options = nla_nest_start (msg, TCA_ACT_OPTIONS)))
+				goto nla_put_failure;
+
+			NLA_PUT (msg, TCA_DEF_PARMS, sizeof (sel), &sel);
+			NLA_PUT (msg, TCA_DEF_DATA, sizeof (simple->sdata), simple->sdata);
+
+			nla_nest_end (msg, act_options);
+
+		} else if (nm_streq (action->kind, NM_PLATFORM_ACTION_KIND_MIRRED)) {
+			const NMPlatformActionMirred *mirred = &action->mirred;
+			struct tc_mirred sel = { 0, };
+
+			if (!(act_options = nla_nest_start (msg, TCA_ACT_OPTIONS)))
+				goto nla_put_failure;
+
+			if (mirred->egress && mirred->redirect)
+				sel.eaction = TCA_EGRESS_REDIR;
+			else if (mirred->egress && mirred->mirror)
+				sel.eaction = TCA_EGRESS_MIRROR;
+			else if (mirred->ingress && mirred->redirect)
+				sel.eaction = TCA_INGRESS_REDIR;
+			else if (mirred->ingress && mirred->mirror)
+				sel.eaction = TCA_INGRESS_MIRROR;
+			sel.ifindex = mirred->ifindex;
+
+			NLA_PUT (msg, TCA_MIRRED_PARMS, sizeof (sel), &sel);
+
+			nla_nest_end (msg, act_options);
+		}
+
+		nla_nest_end (msg, prio);
+	}

	nla_nest_end (msg, tc_options);

--
2.21.0

From a0161aa9772af8d7a35812dfdf86864bb941f751 Mon Sep 17 00:00:00 2001
From: Thomas Haller <thaller@redhat.com>
Date: Wed, 1 May 2019 13:27:28 +0200
Subject: [PATCH 18/20] device/trivial: add space between macro name and
 arguments and vertically align lines

Also calling macros we commonly put a space between the macro name and
the parenthesis.

Also align the parameters. Otherwise it's hard to read for me.

(cherry picked from commit 9399297a82cbb5e8fc218fdefde7005353cb44d6)
---
 src/devices/nm-device.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c
index e2a5fd4bf2..1241a098d1 100644
--- a/src/devices/nm-device.c
+++ b/src/devices/nm-device.c
@@ -6533,14 +6533,14 @@ tc_commit (NMDevice *self)
 } G_STMT_END

			if (strcmp (qdisc->kind, "fq_codel") == 0) {
-				GET_ATTR("limit", qdisc->fq_codel.limit, UINT32, uint32, 0);
-				GET_ATTR("flows", qdisc->fq_codel.flows, UINT32, uint32, 0);
-				GET_ATTR("target", qdisc->fq_codel.target, UINT32, uint32, 0);
-				GET_ATTR("interval", qdisc->fq_codel.interval, UINT32, uint32, 0);
-				GET_ATTR("quantum", qdisc->fq_codel.quantum, UINT32, uint32, 0);
-				GET_ATTR("ce_threshold", qdisc->fq_codel.ce_threshold, UINT32, uint32, NM_PLATFORM_FQ_CODEL_CE_THRESHOLD_DISABLED);
-				GET_ATTR("memory_limit", qdisc->fq_codel.memory_limit, UINT32, uint32, NM_PLATFORM_FQ_CODEL_MEMORY_LIMIT_UNSET);
-				GET_ATTR("ecn", qdisc->fq_codel.ecn, BOOLEAN, boolean, FALSE);
+				GET_ATTR ("limit",        qdisc->fq_codel.limit,        UINT32,  uint32,  0);
+				GET_ATTR ("flows",        qdisc->fq_codel.flows,        UINT32,  uint32,  0);
+				GET_ATTR ("target",       qdisc->fq_codel.target,       UINT32,  uint32,  0);
+				GET_ATTR ("interval",     qdisc->fq_codel.interval,     UINT32,  uint32,  0);
+				GET_ATTR ("quantum",      qdisc->fq_codel.quantum,      UINT32,  uint32,  0);
+				GET_ATTR ("ce_threshold", qdisc->fq_codel.ce_threshold, UINT32,  uint32,  NM_PLATFORM_FQ_CODEL_CE_THRESHOLD_DISABLED);
+				GET_ATTR ("memory_limit", qdisc->fq_codel.memory_limit, UINT32,  uint32,  NM_PLATFORM_FQ_CODEL_MEMORY_LIMIT_UNSET);
+				GET_ATTR ("ecn",          qdisc->fq_codel.ecn,          BOOLEAN, boolean, FALSE);
			}

 #undef GET_ADDR
--
2.21.0

From ea7de52d774f3aa3a099f6fce1cb9313b6456cef Mon Sep 17 00:00:00 2001
From: Thomas Haller <thaller@redhat.com>
Date: Wed, 1 May 2019 16:34:27 +0200
Subject: [PATCH 19/20] device: don't rely on nm_platform_link_get_ifindex()
 returning 0

While nm_platform_link_get_ifindex() is documented to return 0 if the device
is not found, don't rely on it. Instead, check that a valid(!) ifindex was
returned, and only then set the ifindex. Otherwise leave it at zero. There
is of course no difference in practice, but we generally treat invalid ifindexes
as <= 0, so it's not immediately clear what nm_platform_link_get_ifindex()
returns to signal no device.

(cherry picked from commit 9eefe27a1c7879e6f94bbb7dec5c9efe722888fa)
---
 src/devices/nm-device.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c
index 1241a098d1..5195eb667f 100644
--- a/src/devices/nm-device.c
+++ b/src/devices/nm-device.c
@@ -6504,7 +6504,7 @@ tc_commit (NMDevice *self)

	ip_ifindex = nm_device_get_ip_ifindex (self);
	if (!ip_ifindex)
-	       return s_tc == NULL;
+		return s_tc == NULL;

	if (s_tc) {
		nqdiscs = nm_setting_tc_config_get_num_qdiscs (s_tc);
@@ -6591,9 +6591,12 @@ tc_commit (NMDevice *self)

					var = nm_tc_action_get_attribute (action, "dev");
					if (var && g_variant_is_of_type (var, G_VARIANT_TYPE_STRING)) {
-						int ifindex = nm_platform_link_get_ifindex (nm_device_get_platform (self),
-						                                            g_variant_get_string (var, NULL));
-						tfilter->action.mirred.ifindex = ifindex;
+						int ifindex;
+
+						ifindex = nm_platform_link_get_ifindex (nm_device_get_platform (self),
+						                                        g_variant_get_string (var, NULL));
+						if (ifindex > 0)
+							tfilter->action.mirred.ifindex = ifindex;
					}
				}
			}
--
2.21.0

From 6d9030acb60c2f9a89224847f0a5e68f8b55b0f0 Mon Sep 17 00:00:00 2001
From: Thomas Haller <thaller@redhat.com>
Date: Wed, 1 May 2019 16:35:29 +0200
Subject: [PATCH 20/20] device/trivial: add comment about lifetime of "kind" in
 tc_commit()

In general, all fields of public NMPlatform* structs must be
plain/simple. Meaning: copying the struct must be possible without
caring about cloning/duplicating memory.
In other words, if there are fields which lifetime is limited,
then these fields cannot be inside the public part NMPlatform*.

That is why

  - "NMPlatformLink.kind", "NMPlatformQdisc.kind", "NMPlatformTfilter.kind"
    are set by platform code to an interned string (g_intern_string())
    that has a static lifetime.

  - the "ingress_qos_map" field is inside the ref-counted struct NMPObjectLnkVlan
    and not NMPlatformLnkVlan. This field requires managing the lifetime
    of the array and NMPlatformLnkVlan cannot provide that.

See also for example NMPClass.cmd_obj_copy() which can deep-copy an object.
But this is only suitable for fields in NMPObject*. The purpose of this
rule is that you always can safely copy a NMPlatform* struct without
worrying about the ownership and lifetime of the fields (the field's
lifetime is unlimited).

This rule and managing of resource lifetime is the main reason for the
NMPlatform*/NMPObject* split. NMPlatform* structs simply have no mechanism
for copying/releasing fields, that is why the NMPObject* counterpart exists
(which is ref-counted and has a copy and destructor function).

This is violated in tc_commit() for the "kind" strings. The lifetime
of these strings is tied to the setting instance.

We cannot intern the strings (because these are arbitrary strings
and interned strings are leaked indefinitely). We also cannot g_strdup()
the strings, because NMPlatform* is not supposed to own strings.

So, just add comments that warn about this ugliness.

The more correct solution would be to move the "kind" fields inside
NMPObjectQdisc and NMPObjectTfilter, but that is a lot of extra effort.

(cherry picked from commit f2ae994b2359aa690183a268c5b4cc8fb1a6012e)
---
 src/devices/nm-device.c          | 14 +++++++++++++
 src/platform/nm-linux-platform.c |  6 ++++++
 src/platform/nm-platform.c       | 34 ++++++++++++++++++++++++++++++++
 src/platform/nm-platform.h       | 12 +++++++++++
 4 files changed, 66 insertions(+)

diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c
index 5195eb667f..09ba9c5d57 100644
--- a/src/devices/nm-device.c
+++ b/src/devices/nm-device.c
@@ -6516,7 +6516,12 @@ tc_commit (NMDevice *self)
			NMPlatformQdisc *qdisc = NMP_OBJECT_CAST_QDISC (q);

			qdisc->ifindex = ip_ifindex;
+
+			/* Note: kind string is still owned by NMTCTfilter.
+			 * This qdisc instance must not be kept alive beyond this function.
+			 * nm_platform_qdisc_sync() promises to do that. */
			qdisc->kind = nm_tc_qdisc_get_kind (s_qdisc);
+
			qdisc->addr_family = AF_UNSPEC;
			qdisc->handle = nm_tc_qdisc_get_handle (s_qdisc);
			qdisc->parent = nm_tc_qdisc_get_parent (s_qdisc);
@@ -6558,7 +6563,12 @@ tc_commit (NMDevice *self)
			NMPlatformTfilter *tfilter = NMP_OBJECT_CAST_TFILTER (q);

			tfilter->ifindex = ip_ifindex;
+
+			/* Note: kind string is still owned by NMTCTfilter.
+			 * This tfilter instance must not be kept alive beyond this function.
+			 * nm_platform_tfilter_sync() promises to do that. */
			tfilter->kind = nm_tc_tfilter_get_kind (s_tfilter);
+
			tfilter->addr_family = AF_UNSPEC;
			tfilter->handle = nm_tc_tfilter_get_handle (s_tfilter);
			tfilter->parent = nm_tc_tfilter_get_parent (s_tfilter);
@@ -6568,7 +6578,11 @@ tc_commit (NMDevice *self)
			if (action) {
				GVariant *var;

+				/* Note: kind string is still owned by NMTCAction.
+				 * This tfilter instance must not be kept alive beyond this function.
+				 * nm_platform_tfilter_sync() promises to do that. */
				tfilter->action.kind = nm_tc_action_get_kind (action);
+
				if (strcmp (tfilter->action.kind, "simple") == 0) {
					var = nm_tc_action_get_attribute (action, "sdata");
					if (var && g_variant_is_of_type (var, G_VARIANT_TYPE_BYTESTRING)) {
diff --git a/src/platform/nm-linux-platform.c b/src/platform/nm-linux-platform.c
index 4a62a76485..7d66a88fa7 100644
--- a/src/platform/nm-linux-platform.c
+++ b/src/platform/nm-linux-platform.c
@@ -8244,6 +8244,9 @@ qdisc_add (NMPlatform *platform,
	char s_buf[256];
	nm_auto_nlmsg struct nl_msg *msg = NULL;

+	/* Note: @qdisc must not be copied or kept alive because the lifetime of qdisc.kind
+	 * is undefined. */
+
	msg = _nl_msg_new_qdisc (RTM_NEWQDISC, flags, qdisc);

	event_handler_read_netlink (platform, FALSE);
@@ -8285,6 +8288,9 @@ tfilter_add (NMPlatform *platform,
	char s_buf[256];
	nm_auto_nlmsg struct nl_msg *msg = NULL;

+	/* Note: @tfilter must not be copied or kept alive because the lifetime of tfilter.kind
+	 * and tfilter.action.kind is undefined. */
+
	msg = _nl_msg_new_tfilter (RTM_NEWTFILTER, flags, tfilter);

	event_handler_read_netlink (platform, FALSE);
diff --git a/src/platform/nm-platform.c b/src/platform/nm-platform.c
index ee71319dd6..7add7dcdbe 100644
--- a/src/platform/nm-platform.c
+++ b/src/platform/nm-platform.c
@@ -5077,10 +5077,27 @@ nm_platform_qdisc_add (NMPlatform *self,
	int ifindex = qdisc->ifindex;
	_CHECK_SELF (self, klass, -NME_BUG);

+	/* Note: @qdisc must not be copied or kept alive because the lifetime of qdisc.kind
+	 * is undefined. */
+
	_LOG3D ("adding or updating a qdisc: %s", nm_platform_qdisc_to_string (qdisc, NULL, 0));
	return klass->qdisc_add (self, flags, qdisc);
 }

+/**
+ * nm_platform_qdisc_sync:
+ * @self: the #NMPlatform instance
+ * @ifindex: the ifindex where to configure the qdiscs.
+ * @known_qdiscs: the list of qdiscs (#NMPObject).
+ *
+ * The function promises not to take any reference to the qdisc
+ * instances from @known_qdiscs, nor to keep them around after
+ * the function returns. This is important, because it allows the
+ * caller to pass NMPlatformQdisc instances which "kind" string
+ * have a limited lifetime.
+ *
+ * Returns: %TRUE on success.
+ */
 gboolean
 nm_platform_qdisc_sync (NMPlatform *self,
                         int ifindex,
@@ -5143,10 +5160,27 @@ nm_platform_tfilter_add (NMPlatform *self,
	int ifindex = tfilter->ifindex;
	_CHECK_SELF (self, klass, -NME_BUG);

+	/* Note: @tfilter must not be copied or kept alive because the lifetime of tfilter.kind
+	 * and tfilter.action.kind is undefined. */
+
	_LOG3D ("adding or updating a tfilter: %s", nm_platform_tfilter_to_string (tfilter, NULL, 0));
	return klass->tfilter_add (self, flags, tfilter);
 }

+/**
+ * nm_platform_qdisc_sync:
+ * @self: the #NMPlatform instance
+ * @ifindex: the ifindex where to configure the qdiscs.
+ * @known_tfilters: the list of tfilters (#NMPObject).
+ *
+ * The function promises not to take any reference to the tfilter
+ * instances from @known_tfilters, nor to keep them around after
+ * the function returns. This is important, because it allows the
+ * caller to pass NMPlatformTfilter instances which "kind" string
+ * have a limited lifetime.
+ *
+ * Returns: %TRUE on success.
+ */
 gboolean
 nm_platform_tfilter_sync (NMPlatform *self,
                           int ifindex,
diff --git a/src/platform/nm-platform.h b/src/platform/nm-platform.h
index 9b6848d977..a2d8e57ff6 100644
--- a/src/platform/nm-platform.h
+++ b/src/platform/nm-platform.h
@@ -626,7 +626,11 @@ typedef struct {

 typedef struct {
	__NMPlatformObjWithIfindex_COMMON;
+
+	/* beware, kind is embedded in an NMPObject, hence you must
+	 * take care of the lifetime of the string. */
	const char *kind;
+
	int addr_family;
	guint32 handle;
	guint32 parent;
@@ -649,7 +653,11 @@ typedef struct {
 } NMPlatformActionMirred;

 typedef struct {
+
+	/* beware, kind is embedded in an NMPObject, hence you must
+	 * take care of the lifetime of the string. */
	const char *kind;
+
	union {
		NMPlatformActionSimple simple;
		NMPlatformActionMirred mirred;
@@ -661,7 +669,11 @@ typedef struct {

 typedef struct {
	__NMPlatformObjWithIfindex_COMMON;
+
+	/* beware, kind is embedded in an NMPObject, hence you must
+	 * take care of the lifetime of the string. */
	const char *kind;
+
	int addr_family;
	guint32 handle;
	guint32 parent;
--
2.21.0