All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 net-next] rtnetlink: bridge: use ext_ack instead of printk
@ 2017-10-10 11:32 Florian Westphal
  2017-10-10 14:18 ` David Ahern
  0 siblings, 1 reply; 3+ messages in thread
From: Florian Westphal @ 2017-10-10 11:32 UTC (permalink / raw)
  To: netdev; +Cc: Florian Westphal

We can now piggyback error strings to userspace via extended acks
rather than using printk.

Before:
bridge fdb add 01:02:03:04:05:06 dev br0 vlan 4095
RTNETLINK answers: Invalid argument

After:
bridge fdb add 01:02:03:04:05:06 dev br0 vlan 4095
Error: RTM_NEWNEIGH with invalid vlan id.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 v2: forgot to refresh patch, v1 did not even compile.

 net/core/rtnetlink.c | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index e84d108cfee4..19ea53a5210f 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -3066,21 +3066,22 @@ int ndo_dflt_fdb_add(struct ndmsg *ndm,
 }
 EXPORT_SYMBOL(ndo_dflt_fdb_add);
 
-static int fdb_vid_parse(struct nlattr *vlan_attr, u16 *p_vid)
+static int fdb_vid_parse(struct nlattr *vlan_attr, u16 *p_vid,
+			 struct netlink_ext_ack *exta)
 {
 	u16 vid = 0;
 
 	if (vlan_attr) {
 		if (nla_len(vlan_attr) != sizeof(u16)) {
-			pr_info("PF_BRIDGE: RTM_NEWNEIGH with invalid vlan\n");
+			NL_SET_ERR_MSG(exta, "RTM_NEWNEIGH with invalid vlan");
 			return -EINVAL;
 		}
 
 		vid = nla_get_u16(vlan_attr);
 
 		if (!vid || vid >= VLAN_VID_MASK) {
-			pr_info("PF_BRIDGE: RTM_NEWNEIGH with invalid vlan id %d\n",
-				vid);
+			NL_SET_ERR_MSG(exta,
+				       "RTM_NEWNEIGH with invalid vlan id");
 			return -EINVAL;
 		}
 	}
@@ -3089,7 +3090,7 @@ static int fdb_vid_parse(struct nlattr *vlan_attr, u16 *p_vid)
 }
 
 static int rtnl_fdb_add(struct sk_buff *skb, struct nlmsghdr *nlh,
-			struct netlink_ext_ack *extack)
+			struct netlink_ext_ack *exta)
 {
 	struct net *net = sock_net(skb->sk);
 	struct ndmsg *ndm;
@@ -3099,30 +3100,30 @@ static int rtnl_fdb_add(struct sk_buff *skb, struct nlmsghdr *nlh,
 	u16 vid;
 	int err;
 
-	err = nlmsg_parse(nlh, sizeof(*ndm), tb, NDA_MAX, NULL, extack);
+	err = nlmsg_parse(nlh, sizeof(*ndm), tb, NDA_MAX, NULL, exta);
 	if (err < 0)
 		return err;
 
 	ndm = nlmsg_data(nlh);
 	if (ndm->ndm_ifindex == 0) {
-		pr_info("PF_BRIDGE: RTM_NEWNEIGH with invalid ifindex\n");
+		NL_SET_ERR_MSG(exta, "RTM_NEWNEIGH with invalid ifindex");
 		return -EINVAL;
 	}
 
 	dev = __dev_get_by_index(net, ndm->ndm_ifindex);
 	if (dev == NULL) {
-		pr_info("PF_BRIDGE: RTM_NEWNEIGH with unknown ifindex\n");
+		NL_SET_ERR_MSG(exta, "RTM_NEWNEIGH with unknown ifindex");
 		return -ENODEV;
 	}
 
 	if (!tb[NDA_LLADDR] || nla_len(tb[NDA_LLADDR]) != ETH_ALEN) {
-		pr_info("PF_BRIDGE: RTM_NEWNEIGH with invalid address\n");
+		NL_SET_ERR_MSG(exta, "RTM_NEWNEIGH with invalid address");
 		return -EINVAL;
 	}
 
 	addr = nla_data(tb[NDA_LLADDR]);
 
-	err = fdb_vid_parse(tb[NDA_VLAN], &vid);
+	err = fdb_vid_parse(tb[NDA_VLAN], &vid, exta);
 	if (err)
 		return err;
 
@@ -3209,24 +3210,24 @@ static int rtnl_fdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
 
 	ndm = nlmsg_data(nlh);
 	if (ndm->ndm_ifindex == 0) {
-		pr_info("PF_BRIDGE: RTM_DELNEIGH with invalid ifindex\n");
+		NL_SET_ERR_MSG(extack, "RTM_DELNEIGH with invalid ifindex");
 		return -EINVAL;
 	}
 
 	dev = __dev_get_by_index(net, ndm->ndm_ifindex);
 	if (dev == NULL) {
-		pr_info("PF_BRIDGE: RTM_DELNEIGH with unknown ifindex\n");
+		NL_SET_ERR_MSG(extack, "RTM_DELNEIGH with unknown ifindex");
 		return -ENODEV;
 	}
 
 	if (!tb[NDA_LLADDR] || nla_len(tb[NDA_LLADDR]) != ETH_ALEN) {
-		pr_info("PF_BRIDGE: RTM_DELNEIGH with invalid address\n");
+		NL_SET_ERR_MSG(extack, "RTM_DELNEIGH with invalid address");
 		return -EINVAL;
 	}
 
 	addr = nla_data(tb[NDA_LLADDR]);
 
-	err = fdb_vid_parse(tb[NDA_VLAN], &vid);
+	err = fdb_vid_parse(tb[NDA_VLAN], &vid, extack);
 	if (err)
 		return err;
 
@@ -3666,7 +3667,7 @@ static int rtnl_bridge_setlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 
 	dev = __dev_get_by_index(net, ifm->ifi_index);
 	if (!dev) {
-		pr_info("PF_BRIDGE: RTM_SETLINK with unknown ifindex\n");
+		NL_SET_ERR_MSG(extack, "RTM_SETLINK with unknown ifindex");
 		return -ENODEV;
 	}
 
@@ -3741,7 +3742,7 @@ static int rtnl_bridge_dellink(struct sk_buff *skb, struct nlmsghdr *nlh,
 
 	dev = __dev_get_by_index(net, ifm->ifi_index);
 	if (!dev) {
-		pr_info("PF_BRIDGE: RTM_SETLINK with unknown ifindex\n");
+		NL_SET_ERR_MSG(extack, "RTM_SETLINK with unknown ifindex");
 		return -ENODEV;
 	}
 
-- 
2.13.6

^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH v2 net-next] rtnetlink: bridge: use ext_ack instead of printk
  2017-10-10 11:32 [PATCH v2 net-next] rtnetlink: bridge: use ext_ack instead of printk Florian Westphal
@ 2017-10-10 14:18 ` David Ahern
  2017-10-10 14:28   ` Florian Westphal
  0 siblings, 1 reply; 3+ messages in thread
From: David Ahern @ 2017-10-10 14:18 UTC (permalink / raw)
  To: Florian Westphal, netdev

On 10/10/17 5:32 AM, Florian Westphal wrote:
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index e84d108cfee4..19ea53a5210f 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -3066,21 +3066,22 @@ int ndo_dflt_fdb_add(struct ndmsg *ndm,
>  }
>  EXPORT_SYMBOL(ndo_dflt_fdb_add);
>  
> -static int fdb_vid_parse(struct nlattr *vlan_attr, u16 *p_vid)
> +static int fdb_vid_parse(struct nlattr *vlan_attr, u16 *p_vid,
> +			 struct netlink_ext_ack *exta)
>  {
>  	u16 vid = 0;
>  
>  	if (vlan_attr) {
>  		if (nla_len(vlan_attr) != sizeof(u16)) {
> -			pr_info("PF_BRIDGE: RTM_NEWNEIGH with invalid vlan\n");
> +			NL_SET_ERR_MSG(exta, "RTM_NEWNEIGH with invalid vlan");

I realize you are keeping the existing wording, but the messages are
moving from out of line pr_info to inline message in response to a user
command.  From a user's perspective the RTM_NEWNEIGH and DELNEIGH do not
add much value, and the add and del in the bridge command tells which it
is. So in this case just emit "Invalid vlan id".

Although this failure is an invalid vlan attribute as opposed to an
invalid vlan id which is what the next message checks. So the message
needs to be updated as well.


>  			return -EINVAL;
>  		}
>  
>  		vid = nla_get_u16(vlan_attr);
>  
>  		if (!vid || vid >= VLAN_VID_MASK) {
> -			pr_info("PF_BRIDGE: RTM_NEWNEIGH with invalid vlan id %d\n",
> -				vid);
> +			NL_SET_ERR_MSG(exta,
> +				       "RTM_NEWNEIGH with invalid vlan id");
>  			return -EINVAL;
>  		}
>  	}

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH v2 net-next] rtnetlink: bridge: use ext_ack instead of printk
  2017-10-10 14:18 ` David Ahern
@ 2017-10-10 14:28   ` Florian Westphal
  0 siblings, 0 replies; 3+ messages in thread
From: Florian Westphal @ 2017-10-10 14:28 UTC (permalink / raw)
  To: David Ahern; +Cc: Florian Westphal, netdev

David Ahern <dsahern@gmail.com> wrote:
> On 10/10/17 5:32 AM, Florian Westphal wrote:
> > diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> > index e84d108cfee4..19ea53a5210f 100644
> > --- a/net/core/rtnetlink.c
> > +++ b/net/core/rtnetlink.c
> > @@ -3066,21 +3066,22 @@ int ndo_dflt_fdb_add(struct ndmsg *ndm,
> >  }
> >  EXPORT_SYMBOL(ndo_dflt_fdb_add);
> >  
> > -static int fdb_vid_parse(struct nlattr *vlan_attr, u16 *p_vid)
> > +static int fdb_vid_parse(struct nlattr *vlan_attr, u16 *p_vid,
> > +			 struct netlink_ext_ack *exta)
> >  {
> >  	u16 vid = 0;
> >  
> >  	if (vlan_attr) {
> >  		if (nla_len(vlan_attr) != sizeof(u16)) {
> > -			pr_info("PF_BRIDGE: RTM_NEWNEIGH with invalid vlan\n");
> > +			NL_SET_ERR_MSG(exta, "RTM_NEWNEIGH with invalid vlan");
> 
> I realize you are keeping the existing wording, but the messages are
> moving from out of line pr_info to inline message in response to a user
> command.  From a user's perspective the RTM_NEWNEIGH and DELNEIGH do not
> add much value, and the add and del in the bridge command tells which it
> is. So in this case just emit "Invalid vlan id".

Right, makes sense.

> Although this failure is an invalid vlan attribute as opposed to an
> invalid vlan id which is what the next message checks. So the message
> needs to be updated as well.

Indeed, I'll send a v2, thanks!

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-10-10 14:28 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-10 11:32 [PATCH v2 net-next] rtnetlink: bridge: use ext_ack instead of printk Florian Westphal
2017-10-10 14:18 ` David Ahern
2017-10-10 14:28   ` Florian Westphal

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.