[PATCH net-next] team: Use extack to report enslavement failures

[PATCH net-next] team: Use extack to report enslavement failures

[Ido Schimmel]

Use extack inside team's enslavement function and also propagate it to
the netdevice notifier to allow enslaved ports to report the failure
reason. Example:

$ teamd -t team0 -d -c '{"runner": {"name": "lacp"}}'
$ ip link set dev lo master team0
Error: Loopback device can't be added as a team port.

Signed-off-by: Ido Schimmel <idosch@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
---
 drivers/net/team/team.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
index a468439969df..5dd781e65958 100644
--- a/drivers/net/team/team.c
+++ b/drivers/net/team/team.c
@@ -1105,14 +1105,15 @@ static void team_port_disable_netpoll(struct team_port *port)
 }
 #endif
 
-static int team_upper_dev_link(struct team *team, struct team_port *port)
+static int team_upper_dev_link(struct team *team, struct team_port *port,
+			       struct netlink_ext_ack *extack)
 {
 	struct netdev_lag_upper_info lag_upper_info;
 	int err;
 
 	lag_upper_info.tx_type = team->mode->lag_tx_type;
 	err = netdev_master_upper_dev_link(port->dev, team->dev, NULL,
-					   &lag_upper_info, NULL);
+					   &lag_upper_info, extack);
 	if (err)
 		return err;
 	port->dev->priv_flags |= IFF_TEAM_PORT;
@@ -1129,7 +1130,8 @@ static void __team_port_change_port_added(struct team_port *port, bool linkup);
 static int team_dev_type_check_change(struct net_device *dev,
 				      struct net_device *port_dev);
 
-static int team_port_add(struct team *team, struct net_device *port_dev)
+static int team_port_add(struct team *team, struct net_device *port_dev,
+			 struct netlink_ext_ack *extack)
 {
 	struct net_device *dev = team->dev;
 	struct team_port *port;
@@ -1137,12 +1139,14 @@ static int team_port_add(struct team *team, struct net_device *port_dev)
 	int err;
 
 	if (port_dev->flags & IFF_LOOPBACK) {
+		NL_SET_ERR_MSG(extack, "Loopback device can't be added as a team port");
 		netdev_err(dev, "Device %s is loopback device. Loopback devices can't be added as a team port\n",
 			   portname);
 		return -EINVAL;
 	}
 
 	if (team_port_exists(port_dev)) {
+		NL_SET_ERR_MSG(extack, "Device is already a port of a team device");
 		netdev_err(dev, "Device %s is already a port "
 				"of a team device\n", portname);
 		return -EBUSY;
@@ -1150,6 +1154,7 @@ static int team_port_add(struct team *team, struct net_device *port_dev)
 
 	if (port_dev->features & NETIF_F_VLAN_CHALLENGED &&
 	    vlan_uses_dev(dev)) {
+		NL_SET_ERR_MSG(extack, "Device is VLAN challenged and team device has VLAN set up");
 		netdev_err(dev, "Device %s is VLAN challenged and team device has VLAN set up\n",
 			   portname);
 		return -EPERM;
@@ -1160,6 +1165,7 @@ static int team_port_add(struct team *team, struct net_device *port_dev)
 		return err;
 
 	if (port_dev->flags & IFF_UP) {
+		NL_SET_ERR_MSG(extack, "Device is up. Set it down before adding it as a team port");
 		netdev_err(dev, "Device %s is up. Set it down before adding it as a team port\n",
 			   portname);
 		return -EBUSY;
@@ -1227,7 +1233,7 @@ static int team_port_add(struct team *team, struct net_device *port_dev)
 		goto err_handler_register;
 	}
 
-	err = team_upper_dev_link(team, port);
+	err = team_upper_dev_link(team, port, extack);
 	if (err) {
 		netdev_err(dev, "Device %s failed to set upper link\n",
 			   portname);
@@ -1921,7 +1927,7 @@ static int team_add_slave(struct net_device *dev, struct net_device *port_dev,
 	int err;
 
 	mutex_lock(&team->lock);
-	err = team_port_add(team, port_dev);
+	err = team_port_add(team, port_dev, extack);
 	mutex_unlock(&team->lock);
 
 	if (!err)
-- 
2.14.3

Re: [PATCH net-next] team: Use extack to report enslavement failures

[David Ahern]

On 2/27/18 8:38 AM, Ido Schimmel wrote:
> Use extack inside team's enslavement function and also propagate it to
> the netdevice notifier to allow enslaved ports to report the failure
> reason. Example:
> 
> $ teamd -t team0 -d -c '{"runner": {"name": "lacp"}}'
> $ ip link set dev lo master team0
> Error: Loopback device can't be added as a team port.
> 
> Signed-off-by: Ido Schimmel <idosch@mellanox.com>
> Acked-by: Jiri Pirko <jiri@mellanox.com>
> ---
>  drivers/net/team/team.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 

LGTM. Acked-by: David Ahern <dsahern@gmail.com>

Re: [PATCH net-next] team: Use extack to report enslavement failures

[Jiri Benc]

On Tue, 27 Feb 2018 17:38:08 +0200, Ido Schimmel wrote:
>  	if (port_dev->flags & IFF_LOOPBACK) {
> +		NL_SET_ERR_MSG(extack, "Loopback device can't be added as a team port");
>  		netdev_err(dev, "Device %s is loopback device. Loopback devices can't be added as a team port\n",
>  			   portname);

Aren't the netdev_errs unnecessary now?

Or do you keep them for people using old config tools with a new kernel?
If so, for how long? They should certainly be removed eventually. How
do we ensure we don't forget?

Seems to me it would be better to remove them right now.

Thanks,

 Jiri

Re: [PATCH net-next] team: Use extack to report enslavement failures

[Ido Schimmel]

On Tue, Feb 27, 2018 at 08:42:35PM +0100, Jiri Benc wrote:
> On Tue, 27 Feb 2018 17:38:08 +0200, Ido Schimmel wrote:
> >  	if (port_dev->flags & IFF_LOOPBACK) {
> > +		NL_SET_ERR_MSG(extack, "Loopback device can't be added as a team port");
> >  		netdev_err(dev, "Device %s is loopback device. Loopback devices can't be added as a team port\n",
> >  			   portname);
> 
> Aren't the netdev_errs unnecessary now?
> 
> Or do you keep them for people using old config tools with a new kernel?

Yes, that's the intention. I checked bond that was also converted to use
extack and the same thing happens there.

> If so, for how long? They should certainly be removed eventually. How
> do we ensure we don't forget?
> 
> Seems to me it would be better to remove them right now.

I can do that unless someone objects.

Re: [PATCH net-next] team: Use extack to report enslavement failures

[Jakub Kicinski]

On Tue, 27 Feb 2018 22:22:12 +0200, Ido Schimmel wrote:
> > If so, for how long? They should certainly be removed eventually. How
> > do we ensure we don't forget?
> > 
> > Seems to me it would be better to remove them right now.  
> 
> I can do that unless someone objects.

I don't object, but FWIW keep in mind extack errors don't show if
libmnl is not installed..

Re: [PATCH net-next] team: Use extack to report enslavement failures

[Jiri Pirko]

Wed, Feb 28, 2018 at 12:49:59AM CET, kubakici@wp.pl wrote:
>On Tue, 27 Feb 2018 22:22:12 +0200, Ido Schimmel wrote:
>> > If so, for how long? They should certainly be removed eventually. How
>> > do we ensure we don't forget?
>> > 
>> > Seems to me it would be better to remove them right now.  
>> 
>> I can do that unless someone objects.
>
>I don't object, but FWIW keep in mind extack errors don't show if
>libmnl is not installed..

Yeah, or if you have an older iproute2 package. I would keep the existing
dmesg msgs for now. In the future, when everyone is used to exacks, then
we can remove them.

RE: [PATCH net-next] team: Use extack to report enslavement failures

[Yuval Mintz]

> >> > If so, for how long? They should certainly be removed eventually. How
> >> > do we ensure we don't forget?
> >> >
> >> > Seems to me it would be better to remove them right now.
> >>
> >> I can do that unless someone objects.
> >
> >I don't object, but FWIW keep in mind extack errors don't show if
> >libmnl is not installed..
> 
> Yeah, or if you have an older iproute2 package. I would keep the existing
> dmesg msgs for now. In the future, when everyone is used to exacks, then
> we can remove them.

Perhaps it makes sense to introduce netdev_nl_err_msg(dev, extack, msg)
that would do both today and refactor the code to use it?
Later it could be changed to do only NL_SET_ERR_MSG.

Re: [PATCH net-next] team: Use extack to report enslavement failures

[Jiri Benc]

On Wed, 28 Feb 2018 08:12:41 +0100, Jiri Pirko wrote:
> Yeah, or if you have an older iproute2 package. I would keep the existing
> dmesg msgs for now. In the future, when everyone is used to exacks, then
> we can remove them.

How do we ensure that this will actually happen in the future? Usually,
when not done right away, such things tend to be forgotten for years or
even forever.

Also, how distant future are we talking about?

Thanks,

 Jiri

Re: [PATCH net-next] team: Use extack to report enslavement failures

[Jiri Pirko]

Wed, Feb 28, 2018 at 09:58:03AM CET, jbenc@redhat.com wrote:
>On Wed, 28 Feb 2018 08:12:41 +0100, Jiri Pirko wrote:
>> Yeah, or if you have an older iproute2 package. I would keep the existing
>> dmesg msgs for now. In the future, when everyone is used to exacks, then
>> we can remove them.
>
>How do we ensure that this will actually happen in the future? Usually,
>when not done right away, such things tend to be forgotten for years or
>even forever.
>
>Also, how distant future are we talking about?

I would say, quite distant :)
On the other hand, I don't care much about this in particular.

Re: [PATCH net-next] team: Use extack to report enslavement failures

[David Miller]

From: Jiri Pirko <jiri@resnulli.us>
Date: Wed, 28 Feb 2018 08:12:41 +0100

> Wed, Feb 28, 2018 at 12:49:59AM CET, kubakici@wp.pl wrote:
>>On Tue, 27 Feb 2018 22:22:12 +0200, Ido Schimmel wrote:
>>> > If so, for how long? They should certainly be removed eventually. How
>>> > do we ensure we don't forget?
>>> > 
>>> > Seems to me it would be better to remove them right now.  
>>> 
>>> I can do that unless someone objects.
>>
>>I don't object, but FWIW keep in mind extack errors don't show if
>>libmnl is not installed..
> 
> Yeah, or if you have an older iproute2 package. I would keep the existing
> dmesg msgs for now. In the future, when everyone is used to exacks, then
> we can remove them.

That's how I feel about this as well.

Re: [PATCH net-next] team: Use extack to report enslavement failures

[David Miller]

From: Ido Schimmel <idosch@mellanox.com>
Date: Tue, 27 Feb 2018 17:38:08 +0200

> Use extack inside team's enslavement function and also propagate it to
> the netdevice notifier to allow enslaved ports to report the failure
> reason. Example:
> 
> $ teamd -t team0 -d -c '{"runner": {"name": "lacp"}}'
> $ ip link set dev lo master team0
> Error: Loopback device can't be added as a team port.
> 
> Signed-off-by: Ido Schimmel <idosch@mellanox.com>
> Acked-by: Jiri Pirko <jiri@mellanox.com>

Applied, thank you.

David A., when you ACK patches, please start your Acked-by:
tag on the first column of a line.

Don't put things like "LGTM. " before it.

Otherwise patchwork won't automatically pick it up.

Thanks.