diff mbox series

[RFC] add extack errors for iptoken

Message ID 20210331204902.78d87b40@hermes.local (mailing list archive)
State RFC
Delegated to: Netdev Maintainers
Headers show
Series [RFC] add extack errors for iptoken | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Guessed tree name to be net-next
netdev/subject_prefix warning Target tree name not specified in the subject
netdev/cc_maintainers warning 13 maintainers not CCed: zhangkaiheb@126.com dsahern@kernel.org laniel_francis@privacyrequired.com zhudi21@huawei.com andrew@lunn.ch yoshfuji@linux-ipv6.org cong.wang@bytedance.com andriin@fb.com nikolay@nvidia.com colin.king@canonical.com davem@davemloft.net roopa@cumulusnetworks.com kuba@kernel.org
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff fail Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 4433 this patch: 4433
netdev/kdoc success Errors and warnings before: 7 this patch: 7
netdev/verify_fixes success Link
netdev/checkpatch warning WARNING: braces {} are not necessary for single statement blocks WARNING: line length of 82 exceeds 80 columns
netdev/build_allmodconfig_warn success Errors and warnings before: 4632 this patch: 4632
netdev/header_inline success Link

Commit Message

Stephen Hemminger April 1, 2021, 3:49 a.m. UTC 
Perhaps the following (NOT TESTED) kernel patch will show you how such error messages
could be added.

Note: requires trickling down the extack parameter, but that is a good thing because
other place like devconf could use it as well.

Comments

Hongren Zheng April 1, 2021, 10:21 a.m. UTC | #1 
On Wed, Mar 31, 2021 at 08:49:02PM -0700, Stephen Hemminger wrote:
> Perhaps the following (NOT TESTED) kernel patch will show you how such error messages
> could be added.

This is an elegant solution. I found extack is extensively used in 
the other parts of the kernel code for similar purposes.

I also checked the code of iproute2 and found a good support for
extack.

So I am OK with this PATCH.

Also I tested this patch against v5.12-rc5, it compiles and can boot 
with `make ARCH=x86_64 x86_64_defconfig` config in qemu.

I tested it with iproute2 and found a more friendly error prompt:

    $ ip token set ::2 dev enp0s3
    Error: ipv6: Device does accept route adverts.

Tested-by: Hongren Zheng <i@zenithal.me>

> +		NL_SET_ERR_MSG_MOD(extack, "Device does accept route adverts");

Should be "Device does not accept route adverts".
David Ahern April 1, 2021, 2:31 p.m. UTC | #2 
On 3/31/21 9:49 PM, Stephen Hemminger wrote:
> @@ -5681,14 +5682,29 @@ static int inet6_set_iftoken(struct inet6_dev *idev, struct in6_addr *token)
>  
>  	ASSERT_RTNL();
>  
> -	if (!token)
> +	if (!token) {

You forgot to add a message here.

>  		return -EINVAL;
> -	if (dev->flags & (IFF_LOOPBACK | IFF_NOARP))
> +	}
> +
> +	if (dev->flags & IFF_LOOPBACK) {
> +		NL_SET_ERR_MSG_MOD(extack, "Device is loopback");
>  		return -EINVAL;
> -	if (!ipv6_accept_ra(idev))
> +	}
> +
> +	if (dev->flags & IFF_NOARP) {
> +		NL_SET_ERR_MSG_MOD(extack, "Device does not do discovery");

'Device does not do neighbor discovery'

>  		return -EINVAL;
> -	if (idev->cnf.rtr_solicits == 0)
> +	}
> +
> +	if (!ipv6_accept_ra(idev)) {
> +		NL_SET_ERR_MSG_MOD(extack, "Device does accept route adverts");

How about
'Router advertisements are disabled for this device'


> +		return -EINVAL;
> +	}
> +
> +	if (idev->cnf.rtr_solicits == 0) {
> +		NL_SET_ERR_MSG(extack, "Device has disabled router solicitation");

How about

'Router solicitation is disabled on device'
Stephen Hemminger April 1, 2021, 3:06 p.m. UTC | #3 
On Thu, 1 Apr 2021 08:31:05 -0600
David Ahern <dsahern@gmail.com> wrote:

> On 3/31/21 9:49 PM, Stephen Hemminger wrote:
> > @@ -5681,14 +5682,29 @@ static int inet6_set_iftoken(struct inet6_dev *idev, struct in6_addr *token)
> >  
> >  	ASSERT_RTNL();
> >  
> > -	if (!token)
> > +	if (!token) {  
> 
> You forgot to add a message here.

This branch is unreachable, function is  only called from place where it is not NULL.
Hongren Zheng April 28, 2021, 12:55 p.m. UTC | #4 
> Perhaps the following (NOT TESTED) kernel patch will show you how such error messages
> could be added.

Since this patch has been tested, and we have waited a long time for
comments and there is no further response, I wonder if it is the time
to submit this patch to the kernel.
David Ahern April 28, 2021, 3:32 p.m. UTC | #5 
On 4/28/21 6:55 AM, Hongren Zheng wrote:
>> Perhaps the following (NOT TESTED) kernel patch will show you how such error messages
>> could be added.
> 
> Since this patch has been tested, and we have waited a long time for
> comments and there is no further response, I wonder if it is the time
> to submit this patch to the kernel.
> 

send the patch
Hongren Zheng May 29, 2021, 6:31 a.m. UTC | #6 
On Wed, Apr 28, 2021 at 08:55:08PM +0800, Hongren Zheng wrote:
> > Perhaps the following (NOT TESTED) kernel patch will show you how such error messages
> > could be added.
> 
> Since this patch has been tested, and we have waited a long time for
> comments and there is no further response, I wonder if it is the time
> to submit this patch to the kernel.

Is there any updates?

I'm not quite familiar with "RFC" procedure. Should I send this patch to
netdev mailing list with title "[PATCH] add extack errors for iptoken" now
(I suppose not), or wait for Stephen Hemminger sending it, or wait for
more comments?
Stephen Hemminger May 31, 2021, 2:42 a.m. UTC | #7 
On Sat, 29 May 2021 14:31:56 +0800
Hongren Zheng <i@zenithal.me> wrote:

> On Wed, Apr 28, 2021 at 08:55:08PM +0800, Hongren Zheng wrote:
> > > Perhaps the following (NOT TESTED) kernel patch will show you how such error messages
> > > could be added.  
> > 
> > Since this patch has been tested, and we have waited a long time for
> > comments and there is no further response, I wonder if it is the time
> > to submit this patch to the kernel.  
> 
> Is there any updates?
> 
> I'm not quite familiar with "RFC" procedure. Should I send this patch to
> netdev mailing list with title "[PATCH] add extack errors for iptoken" now
> (I suppose not), or wait for Stephen Hemminger sending it, or wait for
> more comments?
> 

The kernel changes is already upstream with this commit for 5.12 kernel

commit 3583a4e8d77d44697a21437227dd53fc6e7b2cb5
Author: Stephen Hemminger <stephen@networkplumber.org>
Date:   Wed Apr 7 08:59:12 2021 -0700

    ipv6: report errors for iftoken via netlink extack
    
    Setting iftoken can fail for several different reasons but there
    and there was no report to user as to the cause. Add netlink
    extended errors to the processing of the request.
    
    This requires adding additional argument through rtnl_af_ops
    set_link_af callback.
    
    Reported-by: Hongren Zheng <li@zenithal.me>
    Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
    Reviewed-by: David Ahern <dsahern@kernel.org>
    Signed-off-by: David S. Miller <davem@davemloft.net>
Hongren Zheng May 31, 2021, 5:26 a.m. UTC | #8 
On Sun, May 30, 2021 at 07:42:34PM -0700, Stephen Hemminger wrote:
> On Sat, 29 May 2021 14:31:56 +0800
> Hongren Zheng <i@zenithal.me> wrote:
> 
> > On Wed, Apr 28, 2021 at 08:55:08PM +0800, Hongren Zheng wrote:
> > > > Perhaps the following (NOT TESTED) kernel patch will show you how such error messages
> > > > could be added.  
> > > 
> > > Since this patch has been tested, and we have waited a long time for
> > > comments and there is no further response, I wonder if it is the time
> > > to submit this patch to the kernel.  
> > 
> > Is there any updates?
> > 
> > I'm not quite familiar with "RFC" procedure. Should I send this patch to
> > netdev mailing list with title "[PATCH] add extack errors for iptoken" now
> > (I suppose not), or wait for Stephen Hemminger sending it, or wait for
> > more comments?
> > 
> 
> The kernel changes is already upstream with this commit for 5.12 kernel
> 
> commit 3583a4e8d77d44697a21437227dd53fc6e7b2cb5
> Author: Stephen Hemminger <stephen@networkplumber.org>
> Date:   Wed Apr 7 08:59:12 2021 -0700
> 
>     ipv6: report errors for iftoken via netlink extack
>     
>     Setting iftoken can fail for several different reasons but there
>     and there was no report to user as to the cause. Add netlink
>     extended errors to the processing of the request.
>     
>     This requires adding additional argument through rtnl_af_ops
>     set_link_af callback.
>     
>     Reported-by: Hongren Zheng <li@zenithal.me>

No wonder I did not receive this email and kept pinging in this thread.

My email address is i@zenithal.me, not li@zenithal.me

Still thank you for getting this upstreamed!

>     Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>     Reviewed-by: David Ahern <dsahern@kernel.org>
>     Signed-off-by: David S. Miller <davem@davemloft.net>
diff mbox series

Patch

diff --git a/include/net/rtnetlink.h b/include/net/rtnetlink.h
index 4da61c950e93..479f60ef54c0 100644
--- a/include/net/rtnetlink.h
+++ b/include/net/rtnetlink.h
@@ -147,8 +147,8 @@  struct rtnl_af_ops {
 	int			(*validate_link_af)(const struct net_device *dev,
 						    const struct nlattr *attr);
 	int			(*set_link_af)(struct net_device *dev,
-					       const struct nlattr *attr);
-
+					       const struct nlattr *attr,
+					       struct netlink_ext_ack *extack);
 	int			(*fill_stats_af)(struct sk_buff *skb,
 						 const struct net_device *dev);
 	size_t			(*get_stats_af_size)(const struct net_device *dev);
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 1bdcb33fb561..3485b16a7ff3 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -2863,7 +2863,7 @@  static int do_setlink(const struct sk_buff *skb,
 
 			BUG_ON(!(af_ops = rtnl_af_lookup(nla_type(af))));
 
-			err = af_ops->set_link_af(dev, af);
+			err = af_ops->set_link_af(dev, af, extack);
 			if (err < 0) {
 				rcu_read_unlock();
 				goto errout;
diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index 75f67994fc85..2e35f68da40a 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -1978,7 +1978,8 @@  static int inet_validate_link_af(const struct net_device *dev,
 	return 0;
 }
 
-static int inet_set_link_af(struct net_device *dev, const struct nlattr *nla)
+static int inet_set_link_af(struct net_device *dev, const struct nlattr *nla,
+			    struct netlink_ext_ack *extack)
 {
 	struct in_device *in_dev = __in_dev_get_rcu(dev);
 	struct nlattr *a, *tb[IFLA_INET_MAX+1];
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 120073ffb666..b817086fbf42 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -5672,7 +5672,8 @@  static int inet6_fill_link_af(struct sk_buff *skb, const struct net_device *dev,
 	return 0;
 }
 
-static int inet6_set_iftoken(struct inet6_dev *idev, struct in6_addr *token)
+static int inet6_set_iftoken(struct inet6_dev *idev, struct in6_addr *token,
+			     struct netlink_ext_ack *extack)
 {
 	struct inet6_ifaddr *ifp;
 	struct net_device *dev = idev->dev;
@@ -5681,14 +5682,29 @@  static int inet6_set_iftoken(struct inet6_dev *idev, struct in6_addr *token)
 
 	ASSERT_RTNL();
 
-	if (!token)
+	if (!token) {
 		return -EINVAL;
-	if (dev->flags & (IFF_LOOPBACK | IFF_NOARP))
+	}
+
+	if (dev->flags & IFF_LOOPBACK) {
+		NL_SET_ERR_MSG_MOD(extack, "Device is loopback");
 		return -EINVAL;
-	if (!ipv6_accept_ra(idev))
+	}
+
+	if (dev->flags & IFF_NOARP) {
+		NL_SET_ERR_MSG_MOD(extack, "Device does not do discovery");
 		return -EINVAL;
-	if (idev->cnf.rtr_solicits == 0)
+	}
+
+	if (!ipv6_accept_ra(idev)) {
+		NL_SET_ERR_MSG_MOD(extack, "Device does accept route adverts");
+		return -EINVAL;
+	}
+
+	if (idev->cnf.rtr_solicits == 0) {
+		NL_SET_ERR_MSG(extack, "Device has disabled router solicitation");
 		return -EINVAL;
+	}
 
 	write_lock_bh(&idev->lock);
 
@@ -5796,7 +5812,8 @@  static int inet6_validate_link_af(const struct net_device *dev,
 	return 0;
 }
 
-static int inet6_set_link_af(struct net_device *dev, const struct nlattr *nla)
+static int inet6_set_link_af(struct net_device *dev, const struct nlattr *nla,
+			     struct netlink_ext_ack *extack)
 {
 	struct inet6_dev *idev = __in6_dev_get(dev);
 	struct nlattr *tb[IFLA_INET6_MAX + 1];
@@ -5809,7 +5826,8 @@  static int inet6_set_link_af(struct net_device *dev, const struct nlattr *nla)
 		BUG();
 
 	if (tb[IFLA_INET6_TOKEN]) {
-		err = inet6_set_iftoken(idev, nla_data(tb[IFLA_INET6_TOKEN]));
+		err = inet6_set_iftoken(idev, nla_data(tb[IFLA_INET6_TOKEN]),
+					extack);
 		if (err)
 			return err;
 	}