Re: [PATCH (repost) net-next] sched: add extack for tfilter_notify

Hangbin Liu
2022-11-29T08:07:14Z

Hi Jamal,

On Tue, Nov 15, 2022 at 11:26:54AM -0500, Jamal Hadi Salim wrote:
> How were you thinking of storing the extack info into the message?
> Note: I wouldnt touch netlink_ack() - maybe write a separate helper for events.

It looks a separate helper is just duplicated codes. Send the notify from
the netlink_ack() is much easy. e.g.

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index a662e8a5ff84..79a1586e8eb0 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -2463,6 +2463,33 @@ netlink_ack_tlv_fill(struct sk_buff *in_skb, struct sk_buff *skb,
(u8 *)extack->miss_nest - (u8 *)nlh));
}

+/**
+ * get_nlgroup - get netlink group id based on nlmsg_type
+ * @nlmsg_type: netlink message type
+ *
+ * Right now, we only support net tc sched. Please add other types if the
+ * sub maintainers feel needed.
+ */
+unsigned int get_nlgroup(__u16 nlmsg_type)
+{
+ switch (nlmsg_type) {
+ case RTM_NEWTFILTER:
+ case RTM_DELTFILTER:
+ case RTM_NEWCHAIN:
+ case RTM_DELCHAIN:
+ case RTM_NEWTCLASS:
+ case RTM_DELTCLASS:
+ case RTM_NEWQDISC:
+ case RTM_DELQDISC:
+ case RTM_GETACTION:
+ case RTM_NEWACTION:
+ case RTM_DELACTION:
+ return RTNLGRP_TC;
+ }
+
+ return 0;
+}
+
void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err,
const struct netlink_ext_ack *extack)
{
@@ -2471,7 +2498,7 @@ void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err,
struct nlmsgerr *errmsg;
size_t payload = sizeof(*errmsg);
struct netlink_sock *nlk = nlk_sk(NETLINK_CB(in_skb).sk);
- unsigned int flags = 0;
+ unsigned int group, flags = 0;
size_t tlvlen;

/* Error messages get the original request appened, unless the user
@@ -2507,7 +2534,9 @@ void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err,

nlmsg_end(skb, rep);

- nlmsg_unicast(in_skb->sk, skb, NETLINK_CB(in_skb).portid);
+ group = get_nlgroup(nlh->nlmsg_type);
+ /* Multicast the error message if we can get group if from nlmsg_type */
+ nlmsg_notify(in_skb->sk, skb, NETLINK_CB(in_skb).portid, group, 1, GFP_KERNEL);
}
EXPORT_SYMBOL(netlink_ack);

What do you think?

Thanks
Hangbin