[PATCHv4 net-next] sched: add new attr TCA_EXT_WARN_MSG to report tc extact message

[PATCHv4 net-next] sched: add new attr TCA_EXT_WARN_MSG to report tc extact message

[Hangbin Liu]

We will report extack message if there is an error via netlink_ack(). But
if the rule is not to be exclusively executed by the hardware, extack is not
passed along and offloading failures don't get logged.

In commit 81c7288b170a ("sched: cls: enable verbose logging") Marcelo
made cls could log verbose info for offloading failures, which helps
improving Open vSwitch debuggability when using flower offloading.

It would also be helpful if userspace monitor tools, like "tc monitor",
could log this kind of message, as it doesn't require vswitchd log level
adjusment. Let's add a new tc attributes to report the extack message so
the monitor program could receive the failures. e.g.

  # tc monitor
  added chain dev enp3s0f1np1 parent ffff: chain 0
  added filter dev enp3s0f1np1 ingress protocol all pref 49152 flower chain 0 handle 0x1
    ct_state +trk+new
    not_in_hw
          action order 1: gact action drop
           random type none pass val 0
           index 1 ref 1 bind 1

  Warning: mlx5_core: matching on ct_state +new isn't supported.

In this patch I only report the extack message on add/del operations.
It doesn't look like we need to report the extack message on get/dump
operations.

Note this message not only reporte to multicast groups, it could also
be reported unicast, which may affect the current usersapce tool's behaivor.

Suggested-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
v4: use specific tc attributes to wrap the warn message
v3: also add this feature for qdisc, class, action, chain notify
v2: use NLMSG_ERROR instad of NLMSG_DONE to report the extack message
---
 include/uapi/linux/rtnetlink.h |  1 +
 net/sched/act_api.c            | 15 +++++---
 net/sched/cls_api.c            | 62 +++++++++++++++++++++-------------
 net/sched/sch_api.c            | 55 ++++++++++++++++++------------
 4 files changed, 84 insertions(+), 49 deletions(-)

diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
index eb2747d58a81..25a0af57dd5e 100644
--- a/include/uapi/linux/rtnetlink.h
+++ b/include/uapi/linux/rtnetlink.h
@@ -635,6 +635,7 @@ enum {
 	TCA_INGRESS_BLOCK,
 	TCA_EGRESS_BLOCK,
 	TCA_DUMP_FLAGS,
+	TCA_EXT_WARN_MSG,
 	__TCA_MAX
 };
 
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 5b3c0ac495be..cd09ef49df22 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -1582,7 +1582,7 @@ int tcf_action_copy_stats(struct sk_buff *skb, struct tc_action *p,
 
 static int tca_get_fill(struct sk_buff *skb, struct tc_action *actions[],
 			u32 portid, u32 seq, u16 flags, int event, int bind,
-			int ref)
+			int ref, struct netlink_ext_ack *extack)
 {
 	struct tcamsg *t;
 	struct nlmsghdr *nlh;
@@ -1606,7 +1606,12 @@ static int tca_get_fill(struct sk_buff *skb, struct tc_action *actions[],
 
 	nla_nest_end(skb, nest);
 
+	if (extack && extack->_msg &&
+	    nla_put_string(skb, TCA_EXT_WARN_MSG, extack->_msg))
+		goto out_nlmsg_trim;
+
 	nlh->nlmsg_len = skb_tail_pointer(skb) - b;
+
 	return skb->len;
 
 out_nlmsg_trim:
@@ -1625,7 +1630,7 @@ tcf_get_notify(struct net *net, u32 portid, struct nlmsghdr *n,
 	if (!skb)
 		return -ENOBUFS;
 	if (tca_get_fill(skb, actions, portid, n->nlmsg_seq, 0, event,
-			 0, 1) <= 0) {
+			 0, 1, NULL) <= 0) {
 		NL_SET_ERR_MSG(extack, "Failed to fill netlink attributes while adding TC action");
 		kfree_skb(skb);
 		return -EINVAL;
@@ -1799,7 +1804,7 @@ tcf_reoffload_del_notify(struct net *net, struct tc_action *action)
 	if (!skb)
 		return -ENOBUFS;
 
-	if (tca_get_fill(skb, actions, 0, 0, 0, RTM_DELACTION, 0, 1) <= 0) {
+	if (tca_get_fill(skb, actions, 0, 0, 0, RTM_DELACTION, 0, 1, NULL) <= 0) {
 		kfree_skb(skb);
 		return -EINVAL;
 	}
@@ -1886,7 +1891,7 @@ tcf_del_notify(struct net *net, struct nlmsghdr *n, struct tc_action *actions[],
 		return -ENOBUFS;
 
 	if (tca_get_fill(skb, actions, portid, n->nlmsg_seq, 0, RTM_DELACTION,
-			 0, 2) <= 0) {
+			 0, 2, extack) <= 0) {
 		NL_SET_ERR_MSG(extack, "Failed to fill netlink TC action attributes");
 		kfree_skb(skb);
 		return -EINVAL;
@@ -1965,7 +1970,7 @@ tcf_add_notify(struct net *net, struct nlmsghdr *n, struct tc_action *actions[],
 		return -ENOBUFS;
 
 	if (tca_get_fill(skb, actions, portid, n->nlmsg_seq, n->nlmsg_flags,
-			 RTM_NEWACTION, 0, 0) <= 0) {
+			 RTM_NEWACTION, 0, 0, extack) <= 0) {
 		NL_SET_ERR_MSG(extack, "Failed to fill netlink attributes while adding TC action");
 		kfree_skb(skb);
 		return -EINVAL;
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 668130f08903..5b4a95e8a1ee 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -488,7 +488,8 @@ static struct tcf_chain *tcf_chain_lookup_rcu(const struct tcf_block *block,
 #endif
 
 static int tc_chain_notify(struct tcf_chain *chain, struct sk_buff *oskb,
-			   u32 seq, u16 flags, int event, bool unicast);
+			   u32 seq, u16 flags, int event, bool unicast,
+			   struct netlink_ext_ack *extack);
 
 static struct tcf_chain *__tcf_chain_get(struct tcf_block *block,
 					 u32 chain_index, bool create,
@@ -521,7 +522,7 @@ static struct tcf_chain *__tcf_chain_get(struct tcf_block *block,
 	 */
 	if (is_first_reference && !by_act)
 		tc_chain_notify(chain, NULL, 0, NLM_F_CREATE | NLM_F_EXCL,
-				RTM_NEWCHAIN, false);
+				RTM_NEWCHAIN, false, NULL);
 
 	return chain;
 
@@ -1817,7 +1818,8 @@ static int tcf_fill_node(struct net *net, struct sk_buff *skb,
 			 struct tcf_proto *tp, struct tcf_block *block,
 			 struct Qdisc *q, u32 parent, void *fh,
 			 u32 portid, u32 seq, u16 flags, int event,
-			 bool terse_dump, bool rtnl_held)
+			 bool terse_dump, bool rtnl_held,
+			 struct netlink_ext_ack *extack)
 {
 	struct tcmsg *tcm;
 	struct nlmsghdr  *nlh;
@@ -1857,7 +1859,13 @@ static int tcf_fill_node(struct net *net, struct sk_buff *skb,
 		    tp->ops->dump(net, tp, fh, skb, tcm, rtnl_held) < 0)
 			goto nla_put_failure;
 	}
+
+	if (extack && extack->_msg &&
+	    nla_put_string(skb, TCA_EXT_WARN_MSG, extack->_msg))
+		goto nla_put_failure;
+
 	nlh->nlmsg_len = skb_tail_pointer(skb) - b;
+
 	return skb->len;
 
 out_nlmsg_trim:
@@ -1871,7 +1879,7 @@ static int tfilter_notify(struct net *net, struct sk_buff *oskb,
 			  struct nlmsghdr *n, struct tcf_proto *tp,
 			  struct tcf_block *block, struct Qdisc *q,
 			  u32 parent, void *fh, int event, bool unicast,
-			  bool rtnl_held)
+			  bool rtnl_held, struct netlink_ext_ack *extack)
 {
 	struct sk_buff *skb;
 	u32 portid = oskb ? NETLINK_CB(oskb).portid : 0;
@@ -1883,7 +1891,7 @@ static int tfilter_notify(struct net *net, struct sk_buff *oskb,
 
 	if (tcf_fill_node(net, skb, tp, block, q, parent, fh, portid,
 			  n->nlmsg_seq, n->nlmsg_flags, event,
-			  false, rtnl_held) <= 0) {
+			  false, rtnl_held, extack) <= 0) {
 		kfree_skb(skb);
 		return -EINVAL;
 	}
@@ -1912,7 +1920,7 @@ static int tfilter_del_notify(struct net *net, struct sk_buff *oskb,
 
 	if (tcf_fill_node(net, skb, tp, block, q, parent, fh, portid,
 			  n->nlmsg_seq, n->nlmsg_flags, RTM_DELTFILTER,
-			  false, rtnl_held) <= 0) {
+			  false, rtnl_held, extack) <= 0) {
 		NL_SET_ERR_MSG(extack, "Failed to build del event notification");
 		kfree_skb(skb);
 		return -EINVAL;
@@ -1938,14 +1946,15 @@ static int tfilter_del_notify(struct net *net, struct sk_buff *oskb,
 static void tfilter_notify_chain(struct net *net, struct sk_buff *oskb,
 				 struct tcf_block *block, struct Qdisc *q,
 				 u32 parent, struct nlmsghdr *n,
-				 struct tcf_chain *chain, int event)
+				 struct tcf_chain *chain, int event,
+				 struct netlink_ext_ack *extack)
 {
 	struct tcf_proto *tp;
 
 	for (tp = tcf_get_next_proto(chain, NULL);
 	     tp; tp = tcf_get_next_proto(chain, tp))
-		tfilter_notify(net, oskb, n, tp, block,
-			       q, parent, NULL, event, false, true);
+		tfilter_notify(net, oskb, n, tp, block, q, parent, NULL,
+			       event, false, true, extack);
 }
 
 static void tfilter_put(struct tcf_proto *tp, void *fh)
@@ -2156,7 +2165,7 @@ static int tc_new_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
 			      flags, extack);
 	if (err == 0) {
 		tfilter_notify(net, skb, n, tp, block, q, parent, fh,
-			       RTM_NEWTFILTER, false, rtnl_held);
+			       RTM_NEWTFILTER, false, rtnl_held, extack);
 		tfilter_put(tp, fh);
 		/* q pointer is NULL for shared blocks */
 		if (q)
@@ -2284,7 +2293,7 @@ static int tc_del_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
 
 	if (prio == 0) {
 		tfilter_notify_chain(net, skb, block, q, parent, n,
-				     chain, RTM_DELTFILTER);
+				     chain, RTM_DELTFILTER, extack);
 		tcf_chain_flush(chain, rtnl_held);
 		err = 0;
 		goto errout;
@@ -2308,7 +2317,7 @@ static int tc_del_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
 
 		tcf_proto_put(tp, rtnl_held, NULL);
 		tfilter_notify(net, skb, n, tp, block, q, parent, fh,
-			       RTM_DELTFILTER, false, rtnl_held);
+			       RTM_DELTFILTER, false, rtnl_held, extack);
 		err = 0;
 		goto errout;
 	}
@@ -2452,7 +2461,7 @@ static int tc_get_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
 		err = -ENOENT;
 	} else {
 		err = tfilter_notify(net, skb, n, tp, block, q, parent,
-				     fh, RTM_NEWTFILTER, true, rtnl_held);
+				     fh, RTM_NEWTFILTER, true, rtnl_held, NULL);
 		if (err < 0)
 			NL_SET_ERR_MSG(extack, "Failed to send filter notify message");
 	}
@@ -2490,7 +2499,7 @@ static int tcf_node_dump(struct tcf_proto *tp, void *n, struct tcf_walker *arg)
 	return tcf_fill_node(net, a->skb, tp, a->block, a->q, a->parent,
 			     n, NETLINK_CB(a->cb->skb).portid,
 			     a->cb->nlh->nlmsg_seq, NLM_F_MULTI,
-			     RTM_NEWTFILTER, a->terse_dump, true);
+			     RTM_NEWTFILTER, a->terse_dump, true, NULL);
 }
 
 static bool tcf_chain_dump(struct tcf_chain *chain, struct Qdisc *q, u32 parent,
@@ -2524,7 +2533,7 @@ static bool tcf_chain_dump(struct tcf_chain *chain, struct Qdisc *q, u32 parent,
 			if (tcf_fill_node(net, skb, tp, block, q, parent, NULL,
 					  NETLINK_CB(cb->skb).portid,
 					  cb->nlh->nlmsg_seq, NLM_F_MULTI,
-					  RTM_NEWTFILTER, false, true) <= 0)
+					  RTM_NEWTFILTER, false, true, NULL) <= 0)
 				goto errout;
 			cb->args[1] = 1;
 		}
@@ -2667,7 +2676,8 @@ static int tc_chain_fill_node(const struct tcf_proto_ops *tmplt_ops,
 			      void *tmplt_priv, u32 chain_index,
 			      struct net *net, struct sk_buff *skb,
 			      struct tcf_block *block,
-			      u32 portid, u32 seq, u16 flags, int event)
+			      u32 portid, u32 seq, u16 flags, int event,
+			      struct netlink_ext_ack *extack)
 {
 	unsigned char *b = skb_tail_pointer(skb);
 	const struct tcf_proto_ops *ops;
@@ -2704,7 +2714,12 @@ static int tc_chain_fill_node(const struct tcf_proto_ops *tmplt_ops,
 			goto nla_put_failure;
 	}
 
+	if (extack && extack->_msg &&
+	    nla_put_string(skb, TCA_EXT_WARN_MSG, extack->_msg))
+		goto out_nlmsg_trim;
+
 	nlh->nlmsg_len = skb_tail_pointer(skb) - b;
+
 	return skb->len;
 
 out_nlmsg_trim:
@@ -2714,7 +2729,8 @@ static int tc_chain_fill_node(const struct tcf_proto_ops *tmplt_ops,
 }
 
 static int tc_chain_notify(struct tcf_chain *chain, struct sk_buff *oskb,
-			   u32 seq, u16 flags, int event, bool unicast)
+			   u32 seq, u16 flags, int event, bool unicast,
+			   struct netlink_ext_ack *extack)
 {
 	u32 portid = oskb ? NETLINK_CB(oskb).portid : 0;
 	struct tcf_block *block = chain->block;
@@ -2728,7 +2744,7 @@ static int tc_chain_notify(struct tcf_chain *chain, struct sk_buff *oskb,
 
 	if (tc_chain_fill_node(chain->tmplt_ops, chain->tmplt_priv,
 			       chain->index, net, skb, block, portid,
-			       seq, flags, event) <= 0) {
+			       seq, flags, event, extack) <= 0) {
 		kfree_skb(skb);
 		return -EINVAL;
 	}
@@ -2756,7 +2772,7 @@ static int tc_chain_notify_delete(const struct tcf_proto_ops *tmplt_ops,
 		return -ENOBUFS;
 
 	if (tc_chain_fill_node(tmplt_ops, tmplt_priv, chain_index, net, skb,
-			       block, portid, seq, flags, RTM_DELCHAIN) <= 0) {
+			       block, portid, seq, flags, RTM_DELCHAIN, NULL) <= 0) {
 		kfree_skb(skb);
 		return -EINVAL;
 	}
@@ -2908,11 +2924,11 @@ static int tc_ctl_chain(struct sk_buff *skb, struct nlmsghdr *n,
 		}
 
 		tc_chain_notify(chain, NULL, 0, NLM_F_CREATE | NLM_F_EXCL,
-				RTM_NEWCHAIN, false);
+				RTM_NEWCHAIN, false, extack);
 		break;
 	case RTM_DELCHAIN:
 		tfilter_notify_chain(net, skb, block, q, parent, n,
-				     chain, RTM_DELTFILTER);
+				     chain, RTM_DELTFILTER, extack);
 		/* Flush the chain first as the user requested chain removal. */
 		tcf_chain_flush(chain, true);
 		/* In case the chain was successfully deleted, put a reference
@@ -2922,7 +2938,7 @@ static int tc_ctl_chain(struct sk_buff *skb, struct nlmsghdr *n,
 		break;
 	case RTM_GETCHAIN:
 		err = tc_chain_notify(chain, skb, n->nlmsg_seq,
-				      n->nlmsg_flags, n->nlmsg_type, true);
+				      n->nlmsg_flags, n->nlmsg_type, true, extack);
 		if (err < 0)
 			NL_SET_ERR_MSG(extack, "Failed to send chain notify message");
 		break;
@@ -3022,7 +3038,7 @@ static int tc_dump_chain(struct sk_buff *skb, struct netlink_callback *cb)
 					 chain->index, net, skb, block,
 					 NETLINK_CB(cb->skb).portid,
 					 cb->nlh->nlmsg_seq, NLM_F_MULTI,
-					 RTM_NEWCHAIN);
+					 RTM_NEWCHAIN, NULL);
 		if (err <= 0)
 			break;
 		index++;
diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index 2317db02c764..88f7d528e3d0 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -902,7 +902,8 @@ static void qdisc_offload_graft_root(struct net_device *dev,
 }
 
 static int tc_fill_qdisc(struct sk_buff *skb, struct Qdisc *q, u32 clid,
-			 u32 portid, u32 seq, u16 flags, int event)
+			 u32 portid, u32 seq, u16 flags, int event,
+			 struct netlink_ext_ack *extack)
 {
 	struct gnet_stats_basic_sync __percpu *cpu_bstats = NULL;
 	struct gnet_stats_queue __percpu *cpu_qstats = NULL;
@@ -970,7 +971,12 @@ static int tc_fill_qdisc(struct sk_buff *skb, struct Qdisc *q, u32 clid,
 	if (gnet_stats_finish_copy(&d) < 0)
 		goto nla_put_failure;
 
+	if (extack && extack->_msg &&
+	    nla_put_string(skb, TCA_EXT_WARN_MSG, extack->_msg))
+		goto out_nlmsg_trim;
+
 	nlh->nlmsg_len = skb_tail_pointer(skb) - b;
+
 	return skb->len;
 
 out_nlmsg_trim:
@@ -991,7 +997,8 @@ static bool tc_qdisc_dump_ignore(struct Qdisc *q, bool dump_invisible)
 
 static int qdisc_notify(struct net *net, struct sk_buff *oskb,
 			struct nlmsghdr *n, u32 clid,
-			struct Qdisc *old, struct Qdisc *new)
+			struct Qdisc *old, struct Qdisc *new,
+			struct netlink_ext_ack *extack)
 {
 	struct sk_buff *skb;
 	u32 portid = oskb ? NETLINK_CB(oskb).portid : 0;
@@ -1002,12 +1009,12 @@ static int qdisc_notify(struct net *net, struct sk_buff *oskb,
 
 	if (old && !tc_qdisc_dump_ignore(old, false)) {
 		if (tc_fill_qdisc(skb, old, clid, portid, n->nlmsg_seq,
-				  0, RTM_DELQDISC) < 0)
+				  0, RTM_DELQDISC, extack) < 0)
 			goto err_out;
 	}
 	if (new && !tc_qdisc_dump_ignore(new, false)) {
 		if (tc_fill_qdisc(skb, new, clid, portid, n->nlmsg_seq,
-				  old ? NLM_F_REPLACE : 0, RTM_NEWQDISC) < 0)
+				  old ? NLM_F_REPLACE : 0, RTM_NEWQDISC, extack) < 0)
 			goto err_out;
 	}
 
@@ -1022,10 +1029,11 @@ static int qdisc_notify(struct net *net, struct sk_buff *oskb,
 
 static void notify_and_destroy(struct net *net, struct sk_buff *skb,
 			       struct nlmsghdr *n, u32 clid,
-			       struct Qdisc *old, struct Qdisc *new)
+			       struct Qdisc *old, struct Qdisc *new,
+			       struct netlink_ext_ack *extack)
 {
 	if (new || old)
-		qdisc_notify(net, skb, n, clid, old, new);
+		qdisc_notify(net, skb, n, clid, old, new, extack);
 
 	if (old)
 		qdisc_put(old);
@@ -1105,12 +1113,12 @@ static int qdisc_graft(struct net_device *dev, struct Qdisc *parent,
 				qdisc_refcount_inc(new);
 			rcu_assign_pointer(dev->qdisc, new ? : &noop_qdisc);
 
-			notify_and_destroy(net, skb, n, classid, old, new);
+			notify_and_destroy(net, skb, n, classid, old, new, extack);
 
 			if (new && new->ops->attach)
 				new->ops->attach(new);
 		} else {
-			notify_and_destroy(net, skb, n, classid, old, new);
+			notify_and_destroy(net, skb, n, classid, old, new, extack);
 		}
 
 		if (dev->flags & IFF_UP)
@@ -1136,7 +1144,7 @@ static int qdisc_graft(struct net_device *dev, struct Qdisc *parent,
 		err = cops->graft(parent, cl, new, &old, extack);
 		if (err)
 			return err;
-		notify_and_destroy(net, skb, n, classid, old, new);
+		notify_and_destroy(net, skb, n, classid, old, new, extack);
 	}
 	return 0;
 }
@@ -1504,7 +1512,7 @@ static int tc_get_qdisc(struct sk_buff *skb, struct nlmsghdr *n,
 		if (err != 0)
 			return err;
 	} else {
-		qdisc_notify(net, skb, n, clid, NULL, q);
+		qdisc_notify(net, skb, n, clid, NULL, q, NULL);
 	}
 	return 0;
 }
@@ -1643,7 +1651,7 @@ static int tc_modify_qdisc(struct sk_buff *skb, struct nlmsghdr *n,
 	}
 	err = qdisc_change(q, tca, extack);
 	if (err == 0)
-		qdisc_notify(net, skb, n, clid, NULL, q);
+		qdisc_notify(net, skb, n, clid, NULL, q, extack);
 	return err;
 
 create_n_graft:
@@ -1710,7 +1718,7 @@ static int tc_dump_qdisc_root(struct Qdisc *root, struct sk_buff *skb,
 		if (!tc_qdisc_dump_ignore(q, dump_invisible) &&
 		    tc_fill_qdisc(skb, q, q->parent, NETLINK_CB(cb->skb).portid,
 				  cb->nlh->nlmsg_seq, NLM_F_MULTI,
-				  RTM_NEWQDISC) <= 0)
+				  RTM_NEWQDISC, NULL) <= 0)
 			goto done;
 		q_idx++;
 	}
@@ -1732,7 +1740,7 @@ static int tc_dump_qdisc_root(struct Qdisc *root, struct sk_buff *skb,
 		if (!tc_qdisc_dump_ignore(q, dump_invisible) &&
 		    tc_fill_qdisc(skb, q, q->parent, NETLINK_CB(cb->skb).portid,
 				  cb->nlh->nlmsg_seq, NLM_F_MULTI,
-				  RTM_NEWQDISC) <= 0)
+				  RTM_NEWQDISC, NULL) <= 0)
 			goto done;
 		q_idx++;
 	}
@@ -1805,8 +1813,8 @@ static int tc_dump_qdisc(struct sk_buff *skb, struct netlink_callback *cb)
  ************************************************/
 
 static int tc_fill_tclass(struct sk_buff *skb, struct Qdisc *q,
-			  unsigned long cl,
-			  u32 portid, u32 seq, u16 flags, int event)
+			  unsigned long cl, u32 portid, u32 seq, u16 flags,
+			  int event, struct netlink_ext_ack *extack)
 {
 	struct tcmsg *tcm;
 	struct nlmsghdr  *nlh;
@@ -1841,7 +1849,12 @@ static int tc_fill_tclass(struct sk_buff *skb, struct Qdisc *q,
 	if (gnet_stats_finish_copy(&d) < 0)
 		goto nla_put_failure;
 
+	if (extack && extack->_msg &&
+	    nla_put_string(skb, TCA_EXT_WARN_MSG, extack->_msg))
+		goto out_nlmsg_trim;
+
 	nlh->nlmsg_len = skb_tail_pointer(skb) - b;
+
 	return skb->len;
 
 out_nlmsg_trim:
@@ -1852,7 +1865,7 @@ static int tc_fill_tclass(struct sk_buff *skb, struct Qdisc *q,
 
 static int tclass_notify(struct net *net, struct sk_buff *oskb,
 			 struct nlmsghdr *n, struct Qdisc *q,
-			 unsigned long cl, int event)
+			 unsigned long cl, int event, struct netlink_ext_ack *extack)
 {
 	struct sk_buff *skb;
 	u32 portid = oskb ? NETLINK_CB(oskb).portid : 0;
@@ -1861,7 +1874,7 @@ static int tclass_notify(struct net *net, struct sk_buff *oskb,
 	if (!skb)
 		return -ENOBUFS;
 
-	if (tc_fill_tclass(skb, q, cl, portid, n->nlmsg_seq, 0, event) < 0) {
+	if (tc_fill_tclass(skb, q, cl, portid, n->nlmsg_seq, 0, event, extack) < 0) {
 		kfree_skb(skb);
 		return -EINVAL;
 	}
@@ -1888,7 +1901,7 @@ static int tclass_del_notify(struct net *net,
 		return -ENOBUFS;
 
 	if (tc_fill_tclass(skb, q, cl, portid, n->nlmsg_seq, 0,
-			   RTM_DELTCLASS) < 0) {
+			   RTM_DELTCLASS, extack) < 0) {
 		kfree_skb(skb);
 		return -EINVAL;
 	}
@@ -2095,7 +2108,7 @@ static int tc_ctl_tclass(struct sk_buff *skb, struct nlmsghdr *n,
 			tc_bind_tclass(q, portid, clid, 0);
 			goto out;
 		case RTM_GETTCLASS:
-			err = tclass_notify(net, skb, n, q, cl, RTM_NEWTCLASS);
+			err = tclass_notify(net, skb, n, q, cl, RTM_NEWTCLASS, extack);
 			goto out;
 		default:
 			err = -EINVAL;
@@ -2113,7 +2126,7 @@ static int tc_ctl_tclass(struct sk_buff *skb, struct nlmsghdr *n,
 	if (cops->change)
 		err = cops->change(q, clid, portid, tca, &new_cl, extack);
 	if (err == 0) {
-		tclass_notify(net, skb, n, q, new_cl, RTM_NEWTCLASS);
+		tclass_notify(net, skb, n, q, new_cl, RTM_NEWTCLASS, extack);
 		/* We just create a new class, need to do reverse binding. */
 		if (cl != new_cl)
 			tc_bind_tclass(q, portid, clid, new_cl);
@@ -2135,7 +2148,7 @@ static int qdisc_class_dump(struct Qdisc *q, unsigned long cl,
 
 	return tc_fill_tclass(a->skb, q, cl, NETLINK_CB(a->cb->skb).portid,
 			      a->cb->nlh->nlmsg_seq, NLM_F_MULTI,
-			      RTM_NEWTCLASS);
+			      RTM_NEWTCLASS, NULL);
 }
 
 static int tc_dump_tclass_qdisc(struct Qdisc *q, struct sk_buff *skb,
-- 
2.38.1

[PATCH iproute2-next 1/2] Revert "tc/tc_monitor: print netlink extack message"

[Hangbin Liu]

This reverts commit 0cc5533b ("tc/tc_monitor: print netlink extack message")
as the commit mentioned is not applied to upstream.

Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 tc/tc_monitor.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/tc/tc_monitor.c b/tc/tc_monitor.c
index 64f31491..b656cfbc 100644
--- a/tc/tc_monitor.c
+++ b/tc/tc_monitor.c
@@ -42,9 +42,6 @@ static int accept_tcmsg(struct rtnl_ctrl_data *ctrl,
 	if (timestamp)
 		print_timestamp(fp);
 
-	if (n->nlmsg_type == NLMSG_DONE)
-		nl_dump_ext_ack_done(n, 0, 0);
-
 	if (n->nlmsg_type == RTM_NEWTFILTER ||
 	    n->nlmsg_type == RTM_DELTFILTER ||
 	    n->nlmsg_type == RTM_NEWCHAIN ||
-- 
2.38.1

[PATCH iproute2-next 2/2] tc: add new attr TCA_EXT_WARN_MSG

[Hangbin Liu]

Currently, when the rule is not to be exclusively executed by the
hardware, extack is not passed along and offloading failures don't
get logged. Add a new attr TCA_EXT_WARN_MSG to log the extack message
so we can monitor the HW failures. e.g.

  # tc monitor
  added chain dev enp3s0f1np1 parent ffff: chain 0
  added filter dev enp3s0f1np1 ingress protocol all pref 49152 flower chain 0 handle 0x1
    ct_state +trk+new
    not_in_hw
          action order 1: gact action drop
           random type none pass val 0
           index 1 ref 1 bind 1

  Warning: mlx5_core: matching on ct_state +new isn't supported.

Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 include/uapi/linux/rtnetlink.h | 1 +
 tc/m_action.c                  | 6 ++++++
 tc/tc_filter.c                 | 5 +++++
 tc/tc_qdisc.c                  | 6 ++++++
 4 files changed, 18 insertions(+)

diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
index f4a540c0..217b25b9 100644
--- a/include/uapi/linux/rtnetlink.h
+++ b/include/uapi/linux/rtnetlink.h
@@ -635,6 +635,7 @@ enum {
 	TCA_INGRESS_BLOCK,
 	TCA_EGRESS_BLOCK,
 	TCA_DUMP_FLAGS,
+	TCA_EXT_WARN_MSG,
 	__TCA_MAX
 };
 
diff --git a/tc/m_action.c b/tc/m_action.c
index b3fd0193..7121c2fb 100644
--- a/tc/m_action.c
+++ b/tc/m_action.c
@@ -590,6 +590,12 @@ int print_action(struct nlmsghdr *n, void *arg)
 
 	open_json_object(NULL);
 	tc_dump_action(fp, tb[TCA_ACT_TAB], tot_acts ? *tot_acts:0, false);
+
+	if (tb[TCA_EXT_WARN_MSG]) {
+		print_string(PRINT_ANY, "Warn", "%s ", rta_getattr_str(tb[TCA_EXT_WARN_MSG]));
+		print_nl();
+	}
+
 	close_json_object();
 
 	return 0;
diff --git a/tc/tc_filter.c b/tc/tc_filter.c
index 71be2e81..dac74f58 100644
--- a/tc/tc_filter.c
+++ b/tc/tc_filter.c
@@ -371,6 +371,11 @@ int print_filter(struct nlmsghdr *n, void *arg)
 		print_nl();
 	}
 
+	if (tb[TCA_EXT_WARN_MSG]) {
+		print_string(PRINT_ANY, "Warn", "%s ", rta_getattr_str(tb[TCA_EXT_WARN_MSG]));
+		print_nl();
+	}
+
 	close_json_object();
 	fflush(fp);
 	return 0;
diff --git a/tc/tc_qdisc.c b/tc/tc_qdisc.c
index 33a6665e..a84602b4 100644
--- a/tc/tc_qdisc.c
+++ b/tc/tc_qdisc.c
@@ -346,6 +346,12 @@ int print_qdisc(struct nlmsghdr *n, void *arg)
 			print_nl();
 		}
 	}
+
+	if (tb[TCA_EXT_WARN_MSG]) {
+		print_string(PRINT_ANY, "Warn", "%s ", rta_getattr_str(tb[TCA_EXT_WARN_MSG]));
+		print_nl();
+	}
+
 	close_json_object();
 	fflush(fp);
 	return 0;
-- 
2.38.1

Re: [PATCH iproute2-next 2/2] tc: add new attr TCA_EXT_WARN_MSG

[Hangbin Liu]

Hi David,

Oh, I forgot to add --no-thread when send this patch. Sorry if this makes
any trouble for you.

Hangbin

On Fri, Jan 13, 2023 at 11:46:17AM +0800, Hangbin Liu wrote:
> Currently, when the rule is not to be exclusively executed by the
> hardware, extack is not passed along and offloading failures don't
> get logged. Add a new attr TCA_EXT_WARN_MSG to log the extack message
> so we can monitor the HW failures. e.g.
> 
>   # tc monitor
>   added chain dev enp3s0f1np1 parent ffff: chain 0
>   added filter dev enp3s0f1np1 ingress protocol all pref 49152 flower chain 0 handle 0x1
>     ct_state +trk+new
>     not_in_hw
>           action order 1: gact action drop
>            random type none pass val 0
>            index 1 ref 1 bind 1
> 
>   Warning: mlx5_core: matching on ct_state +new isn't supported.
> 
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
>  include/uapi/linux/rtnetlink.h | 1 +
>  tc/m_action.c                  | 6 ++++++
>  tc/tc_filter.c                 | 5 +++++
>  tc/tc_qdisc.c                  | 6 ++++++
>  4 files changed, 18 insertions(+)
> 
> diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
> index f4a540c0..217b25b9 100644
> --- a/include/uapi/linux/rtnetlink.h
> +++ b/include/uapi/linux/rtnetlink.h
> @@ -635,6 +635,7 @@ enum {
>  	TCA_INGRESS_BLOCK,
>  	TCA_EGRESS_BLOCK,
>  	TCA_DUMP_FLAGS,
> +	TCA_EXT_WARN_MSG,
>  	__TCA_MAX
>  };
>  
> diff --git a/tc/m_action.c b/tc/m_action.c
> index b3fd0193..7121c2fb 100644
> --- a/tc/m_action.c
> +++ b/tc/m_action.c
> @@ -590,6 +590,12 @@ int print_action(struct nlmsghdr *n, void *arg)
>  
>  	open_json_object(NULL);
>  	tc_dump_action(fp, tb[TCA_ACT_TAB], tot_acts ? *tot_acts:0, false);
> +
> +	if (tb[TCA_EXT_WARN_MSG]) {
> +		print_string(PRINT_ANY, "Warn", "%s ", rta_getattr_str(tb[TCA_EXT_WARN_MSG]));
> +		print_nl();
> +	}
> +
>  	close_json_object();
>  
>  	return 0;
> diff --git a/tc/tc_filter.c b/tc/tc_filter.c
> index 71be2e81..dac74f58 100644
> --- a/tc/tc_filter.c
> +++ b/tc/tc_filter.c
> @@ -371,6 +371,11 @@ int print_filter(struct nlmsghdr *n, void *arg)
>  		print_nl();
>  	}
>  
> +	if (tb[TCA_EXT_WARN_MSG]) {
> +		print_string(PRINT_ANY, "Warn", "%s ", rta_getattr_str(tb[TCA_EXT_WARN_MSG]));
> +		print_nl();
> +	}
> +
>  	close_json_object();
>  	fflush(fp);
>  	return 0;
> diff --git a/tc/tc_qdisc.c b/tc/tc_qdisc.c
> index 33a6665e..a84602b4 100644
> --- a/tc/tc_qdisc.c
> +++ b/tc/tc_qdisc.c
> @@ -346,6 +346,12 @@ int print_qdisc(struct nlmsghdr *n, void *arg)
>  			print_nl();
>  		}
>  	}
> +
> +	if (tb[TCA_EXT_WARN_MSG]) {
> +		print_string(PRINT_ANY, "Warn", "%s ", rta_getattr_str(tb[TCA_EXT_WARN_MSG]));
> +		print_nl();
> +	}
> +
>  	close_json_object();
>  	fflush(fp);
>  	return 0;
> -- 
> 2.38.1
> 

Re: [PATCH iproute2-next 2/2] tc: add new attr TCA_EXT_WARN_MSG

[Stephen Hemminger]

On Fri, 13 Jan 2023 11:46:17 +0800
Hangbin Liu <liuhangbin@gmail.com> wrote:

> 	if (tb[TCA_EXT_WARN_MSG]) {
> +		print_string(PRINT_ANY, "Warn", "%s ", rta_getattr_str(tb[TCA_EXT_WARN_MSG]));
> +		print_nl();
> +	}
> +

Errors should go to stderr, and not to stdout. And no JSON support please.

Re: [PATCH iproute2-next 2/2] tc: add new attr TCA_EXT_WARN_MSG

[Hangbin Liu]

On Thu, Jan 12, 2023 at 08:30:19PM -0800, Stephen Hemminger wrote:
> On Fri, 13 Jan 2023 11:46:17 +0800
> Hangbin Liu <liuhangbin@gmail.com> wrote:
> 
> > 	if (tb[TCA_EXT_WARN_MSG]) {
> > +		print_string(PRINT_ANY, "Warn", "%s ", rta_getattr_str(tb[TCA_EXT_WARN_MSG]));
> > +		print_nl();
> > +	}
> > +
> 
> Errors should go to stderr, and not to stdout. And no JSON support please.

Thanks, I thought this belongs to an attr data. So I put it in the JSON object.
I will adjust this to stderr and out side of JSON.

Hangbin

Re: [PATCH iproute2-next 2/2] tc: add new attr TCA_EXT_WARN_MSG

[David Ahern]

On 1/12/23 8:46 PM, Hangbin Liu wrote:
> diff --git a/tc/tc_qdisc.c b/tc/tc_qdisc.c
> index 33a6665e..a84602b4 100644
> --- a/tc/tc_qdisc.c
> +++ b/tc/tc_qdisc.c
> @@ -346,6 +346,12 @@ int print_qdisc(struct nlmsghdr *n, void *arg)
>  			print_nl();
>  		}
>  	}
> +
> +	if (tb[TCA_EXT_WARN_MSG]) {
> +		print_string(PRINT_ANY, "Warn", "%s ", rta_getattr_str(tb[TCA_EXT_WARN_MSG]));
> +		print_nl();
> +	}
> +

given the repetition with the WARN_MSG, how about a helper in tc_util.c

Re: [PATCHv4 net-next] sched: add new attr TCA_EXT_WARN_MSG to report tc extact message

[Jakub Kicinski]

On Fri, 13 Jan 2023 11:43:53 +0800 Hangbin Liu wrote:
> We will report extack message if there is an error via netlink_ack(). But
> if the rule is not to be exclusively executed by the hardware, extack is not
> passed along and offloading failures don't get logged.

Acked-by: Jakub Kicinski <kuba@kernel.org>

Re: [PATCHv4 net-next] sched: add new attr TCA_EXT_WARN_MSG to report tc extact message

[Jamal Hadi Salim]

Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

On Sat, Jan 14, 2023 at 1:15 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Fri, 13 Jan 2023 11:43:53 +0800 Hangbin Liu wrote:
> > We will report extack message if there is an error via netlink_ack(). But
> > if the rule is not to be exclusively executed by the hardware, extack is not
> > passed along and offloading failures don't get logged.
>
> Acked-by: Jakub Kicinski <kuba@kernel.org>

Re: [PATCH iproute2-next 2/2] tc: add new attr TCA_EXT_WARN_MSG

[Jamal Hadi Salim]

This is not really an error IMO (and therefore doesnt belong in
stderr). If i send a request to add an
entry and ask that it is installed both in the kernel as well as
offloaded into h/w and half of that
worked (example hardware rejected it for some reason) then the event
generated (as observed by
f.e. tc mon) will appear in TCA_EXT_WARN_MSG and the consumer of that
event needs to see it
if they are using the json format.

cheers,
jamal

On Thu, Jan 12, 2023 at 11:30 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> On Fri, 13 Jan 2023 11:46:17 +0800
> Hangbin Liu <liuhangbin@gmail.com> wrote:
>
> >       if (tb[TCA_EXT_WARN_MSG]) {
> > +             print_string(PRINT_ANY, "Warn", "%s ", rta_getattr_str(tb[TCA_EXT_WARN_MSG]));
> > +             print_nl();
> > +     }
> > +
>
> Errors should go to stderr, and not to stdout. And no JSON support please.

Re: [PATCH iproute2-next 2/2] tc: add new attr TCA_EXT_WARN_MSG

[Stephen Hemminger]

On Sat, 14 Jan 2023 07:59:39 -0500
Jamal Hadi Salim <jhs@mojatatu.com> wrote:

> This is not really an error IMO (and therefore doesnt belong in
> stderr). If i send a request to add an
> entry and ask that it is installed both in the kernel as well as
> offloaded into h/w and half of that
> worked (example hardware rejected it for some reason) then the event
> generated (as observed by
> f.e. tc mon) will appear in TCA_EXT_WARN_MSG and the consumer of that
> event needs to see it
> if they are using the json format.
> 

Ok, but use lower case for JSON tag following existing conventions.

Note: json support in monitor mode is incomplete for many of the
commands bridge, ip, tc, devlink. It doesn't always generate valid JSON
yet.

[PATCHv2 iproute2-next 0/2] tc: add new attr TCA_EXT_WARN_MSG

[Hangbin Liu]

This patch set revert commit 0cc5533b ("tc/tc_monitor: print netlink extack
message") as we never used it. Then add new attr TCA_EXT_WARN_MSG to print
the extack message as we proposed in the net-next patch.

I would reply to the kernel patch directly as it's not updated.

v2: Add a helper to print the warn message. I still print the msg in
json ojbect given the disscuss in
https://lore.kernel.org/all/20230114090311.1adf0176@hermes.local/

Hangbin Liu (2):
  Revert "tc/tc_monitor: print netlink extack message"
  tc: add new attr TCA_EXT_WARN_MSG

 include/uapi/linux/rtnetlink.h | 1 +
 tc/m_action.c                  | 1 +
 tc/tc_filter.c                 | 1 +
 tc/tc_monitor.c                | 3 ---
 tc/tc_qdisc.c                  | 2 ++
 tc/tc_util.c                   | 9 +++++++++
 tc/tc_util.h                   | 2 ++
 7 files changed, 16 insertions(+), 3 deletions(-)

-- 
2.38.1

[PATCHv2 iproute2-next 1/2] Revert "tc/tc_monitor: print netlink extack message"

[Hangbin Liu]

This reverts commit 0cc5533b ("tc/tc_monitor: print netlink extack message")
as the commit mentioned is not applied to upstream.

Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
v2: no update
---
 tc/tc_monitor.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/tc/tc_monitor.c b/tc/tc_monitor.c
index 64f31491..b656cfbc 100644
--- a/tc/tc_monitor.c
+++ b/tc/tc_monitor.c
@@ -42,9 +42,6 @@ static int accept_tcmsg(struct rtnl_ctrl_data *ctrl,
 	if (timestamp)
 		print_timestamp(fp);
 
-	if (n->nlmsg_type == NLMSG_DONE)
-		nl_dump_ext_ack_done(n, 0, 0);
-
 	if (n->nlmsg_type == RTM_NEWTFILTER ||
 	    n->nlmsg_type == RTM_DELTFILTER ||
 	    n->nlmsg_type == RTM_NEWCHAIN ||
-- 
2.38.1

[PATCHv2 iproute2-next 2/2] tc: add new attr TCA_EXT_WARN_MSG

[Hangbin Liu]

Currently, when the rule is not to be exclusively executed by the
hardware, extack is not passed along and offloading failures don't
get logged. Add a new attr TCA_EXT_WARN_MSG to log the extack message
so we can monitor the HW failures. e.g.

  # tc monitor
  added chain dev enp3s0f1np1 parent ffff: chain 0
  added filter dev enp3s0f1np1 ingress protocol all pref 49152 flower chain 0 handle 0x1
    ct_state +trk+new
    not_in_hw
          action order 1: gact action drop
           random type none pass val 0
           index 1 ref 1 bind 1

  mlx5_core: matching on ct_state +new isn't supported.

Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
v2: Add a helper to print the warn message. I still print the msg in
json ojbect given the disscuss in
https://lore.kernel.org/all/20230114090311.1adf0176@hermes.local/
---
 include/uapi/linux/rtnetlink.h | 1 +
 tc/m_action.c                  | 1 +
 tc/tc_filter.c                 | 1 +
 tc/tc_qdisc.c                  | 2 ++
 tc/tc_util.c                   | 9 +++++++++
 tc/tc_util.h                   | 2 ++
 6 files changed, 16 insertions(+)

diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
index f4a540c0..217b25b9 100644
--- a/include/uapi/linux/rtnetlink.h
+++ b/include/uapi/linux/rtnetlink.h
@@ -635,6 +635,7 @@ enum {
 	TCA_INGRESS_BLOCK,
 	TCA_EGRESS_BLOCK,
 	TCA_DUMP_FLAGS,
+	TCA_EXT_WARN_MSG,
 	__TCA_MAX
 };
 
diff --git a/tc/m_action.c b/tc/m_action.c
index b3fd0193..b45c4936 100644
--- a/tc/m_action.c
+++ b/tc/m_action.c
@@ -590,6 +590,7 @@ int print_action(struct nlmsghdr *n, void *arg)
 
 	open_json_object(NULL);
 	tc_dump_action(fp, tb[TCA_ACT_TAB], tot_acts ? *tot_acts:0, false);
+	print_ext_msg(tb);
 	close_json_object();
 
 	return 0;
diff --git a/tc/tc_filter.c b/tc/tc_filter.c
index 71be2e81..cfc72c00 100644
--- a/tc/tc_filter.c
+++ b/tc/tc_filter.c
@@ -371,6 +371,7 @@ int print_filter(struct nlmsghdr *n, void *arg)
 		print_nl();
 	}
 
+	print_ext_msg(tb);
 	close_json_object();
 	fflush(fp);
 	return 0;
diff --git a/tc/tc_qdisc.c b/tc/tc_qdisc.c
index 33a6665e..faa8daed 100644
--- a/tc/tc_qdisc.c
+++ b/tc/tc_qdisc.c
@@ -346,6 +346,8 @@ int print_qdisc(struct nlmsghdr *n, void *arg)
 			print_nl();
 		}
 	}
+
+	print_ext_msg(tb);
 	close_json_object();
 	fflush(fp);
 	return 0;
diff --git a/tc/tc_util.c b/tc/tc_util.c
index d2622063..aee25f0b 100644
--- a/tc/tc_util.c
+++ b/tc/tc_util.c
@@ -848,3 +848,12 @@ void print_masked_be16(const char *name, struct rtattr *attr,
 	print_masked_type(UINT16_MAX, __rta_getattr_be16_u32, name, attr,
 			  mask_attr, newline);
 }
+
+void print_ext_msg(struct rtattr **tb)
+{
+	if (!tb[TCA_EXT_WARN_MSG])
+		return;
+
+	print_string(PRINT_ANY, "warn", "%s", rta_getattr_str(tb[TCA_EXT_WARN_MSG]));
+	print_nl();
+}
diff --git a/tc/tc_util.h b/tc/tc_util.h
index a3fa7360..c535dccb 100644
--- a/tc/tc_util.h
+++ b/tc/tc_util.h
@@ -133,4 +133,6 @@ void print_masked_u8(const char *name, struct rtattr *attr,
 		     struct rtattr *mask_attr, bool newline);
 void print_masked_be16(const char *name, struct rtattr *attr,
 		       struct rtattr *mask_attr, bool newline);
+
+void print_ext_msg(struct rtattr **tb);
 #endif
-- 
2.38.1

Re: [PATCHv4 net-next] sched: add new attr TCA_EXT_WARN_MSG to report tc extact message

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (master)
by Paolo Abeni <pabeni@redhat.com>:

On Fri, 13 Jan 2023 11:43:53 +0800 you wrote:
> We will report extack message if there is an error via netlink_ack(). But
> if the rule is not to be exclusively executed by the hardware, extack is not
> passed along and offloading failures don't get logged.
> 
> In commit 81c7288b170a ("sched: cls: enable verbose logging") Marcelo
> made cls could log verbose info for offloading failures, which helps
> improving Open vSwitch debuggability when using flower offloading.
> 
> [...]

Here is the summary with links:
  - [PATCHv4,net-next] sched: add new attr TCA_EXT_WARN_MSG to report tc extact message
    https://git.kernel.org/netdev/net-next/c/0349b8779cc9

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH iproute2-next 2/2] tc: add new attr TCA_EXT_WARN_MSG

[Jamal Hadi Salim]

On Sat, Jan 14, 2023 at 12:03 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> On Sat, 14 Jan 2023 07:59:39 -0500
> Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>

[..]
>
> Ok, but use lower case for JSON tag following existing conventions.
>
> Note: json support in monitor mode is incomplete for many of the
> commands bridge, ip, tc, devlink. It doesn't always generate valid JSON
> yet.

We can work for starters with the tc one and maybe cover ip as well...

cheers,
jamal

Re: [PATCHv2 iproute2-next 0/2] tc: add new attr TCA_EXT_WARN_MSG

[patchwork-bot+netdevbpf]

Hello:

This series was applied to iproute2/iproute2-next.git (main)
by David Ahern <dsahern@kernel.org>:

On Tue, 17 Jan 2023 15:19:23 +0800 you wrote:
> This patch set revert commit 0cc5533b ("tc/tc_monitor: print netlink extack
> message") as we never used it. Then add new attr TCA_EXT_WARN_MSG to print
> the extack message as we proposed in the net-next patch.
> 
> I would reply to the kernel patch directly as it's not updated.
> 
> v2: Add a helper to print the warn message. I still print the msg in
> json ojbect given the disscuss in
> https://lore.kernel.org/all/20230114090311.1adf0176@hermes.local/
> 
> [...]

Here is the summary with links:
  - [PATCHv2,iproute2-next,1/2] Revert "tc/tc_monitor: print netlink extack message"
    https://git.kernel.org/pub/scm/network/iproute2/iproute2-next.git/commit/?id=77d4425560ce
  - [PATCHv2,iproute2-next,2/2] tc: add new attr TCA_EXT_WARN_MSG
    (no matching commit)

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH iproute2-next 2/2] tc: add new attr TCA_EXT_WARN_MSG

[Hangbin Liu]

On Tue, Jan 17, 2023 at 10:09:41AM -0500, Jamal Hadi Salim wrote:
> > Ok, but use lower case for JSON tag following existing conventions.
> >
> > Note: json support in monitor mode is incomplete for many of the
> > commands bridge, ip, tc, devlink. It doesn't always generate valid JSON
> > yet.
> 
> We can work for starters with the tc one and maybe cover ip as well...

I tried adding the JSON obj for ip monitor. There are 2 choices and some
issues.

If we open JSON before/after accept_msg:

diff --git a/ip/ipmonitor.c b/ip/ipmonitor.c
index 9b055264..a6944636 100644
--- a/ip/ipmonitor.c
+++ b/ip/ipmonitor.c
@@ -338,8 +338,13 @@ int do_ipmonitor(int argc, char **argv)
        netns_nsid_socket_init();
        netns_map_init();

-       if (rtnl_listen(&rth, accept_msg, stdout) < 0)
+       new_json_obj(json);
+       if (rtnl_listen(&rth, accept_msg, stdout) < 0) {
+               delete_json_obj();
                exit(2);
+       }
+
+       delete_json_obj();

        return 0;
 }

The JSON output looks like the following result, the ifindex is not quoted in
the braces. And the JSON format is not complete at the end.
# ip/ip -j -p monitor
[ {
        "family": "inet",
        "interface": "veth0",
        "forwarding": false,
        "rp_filter": "strict",
        "mc_forwarding": false,
        "proxy_neigh": false,
        "ignore_routes_with_linkdown": false
    },{
        "family": "inet6",
        "interface": "veth0",
        "forwarding": false,
        "mc_forwarding": false,
        "proxy_neigh": false,
        "ignore_routes_with_linkdown": false
    },
    "ifindex": 17,
    "link": null,
    "ifname": "veth0",
    "flags": [ "BROADCAST","MULTICAST" ],
    "mtu": 1500,
    "qdisc": "noop",
    "operstate": "DOWN",
    "group": "default",
    "link_type": "ether",
    "address": "72:4f:8c:32:13:63",
    "broadcast": "ff:ff:ff:ff:ff:ff",{
        "family": "inet",
        "interface": "veth1",
        "forwarding": false,
        "rp_filter": "strict",
        "mc_forwarding": false,
        "proxy_neigh": false,
        "ignore_routes_with_linkdown": false
    },{
        "family": "inet6",
        "interface": "veth1",
        "forwarding": false,
        "mc_forwarding": false,
        "proxy_neigh": false,
        "ignore_routes_with_linkdown": false
    },
    "ifindex": 18,
    "link": "veth0",
    "ifname": "veth1",
    "flags": [ "BROADCAST","MULTICAST","M-DOWN" ],
    "mtu": 1500,
    "qdisc": "noop",
    "operstate": "DOWN",
    "group": "default",
    "link_type": "ether",
    "address": "42:75:b7:38:9c:ad",
    "broadcast": "ff:ff:ff:ff:ff:ff",{
        "dst": "fe80::ee3e:f701:b990:8a61",
        "dev": "eth0",
        "lladdr": "ec:3e:f7:90:8a:61",
        "router": null,
        "state": [ "STALE" ]
    }^C


If we open JSON in the accept_msg(). Like:

diff --git a/ip/ipmonitor.c b/ip/ipmonitor.c
index 9b055264..4af18a6c 100644
--- a/ip/ipmonitor.c
+++ b/ip/ipmonitor.c
@@ -55,6 +55,7 @@ static int accept_msg(struct rtnl_ctrl_data *ctrl,
 {
        FILE *fp = (FILE *)arg;

+       new_json_obj(json);
        switch (n->nlmsg_type) {
        case RTM_NEWROUTE:
        case RTM_DELROUTE: {
@@ -63,21 +64,21 @@ static int accept_msg(struct rtnl_ctrl_data *ctrl,

                if (len < 0) {
                        fprintf(stderr, "BUG: wrong nlmsg len %d\n", len);
-                       return -1;
+                       goto err_out;
                }

                if (r->rtm_flags & RTM_F_CLONED)
-                       return 0;
+                       goto out;

[...]

@@ -170,7 +171,14 @@ static int accept_msg(struct rtnl_ctrl_data *ctrl,
                        n->nlmsg_flags, n->nlmsg_flags, n->nlmsg_len,
                        n->nlmsg_len);
        }
+
+out:
+       delete_json_obj();
        return 0;
+
+err_out:
+       delete_json_obj();
+       return -1;
 }

The result would like:

# ./ip/ip -j monitor
[{"deleted":true,"family":"inet","interface":"veth0"}]
[{"deleted":true,"family":"inet6","interface":"veth0"}]
[{"deleted":true,"family":"inet","interface":"veth1"}]
[{"deleted":true,"family":"inet6","interface":"veth1"}]

This format looks good, but I'm not sure if the JSON output is valid with
list for each line.

Any comments?

Thanks
Hangbin