| 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 |
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".
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'
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.
> 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.
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
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?
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>
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 --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; }