Re: [PATCHi next] veth: advertise peer link relationship for both devices

[Lance Richardson <lrichard@redhat.com>]

----- Original Message -----
> From: "David Miller" <davem@davemloft.net>
> To: lrichard@redhat.com
> Cc: netdev@vger.kernel.org, "nicolas dichtel" <nicolas.dichtel@6wind.com>
> Sent: Saturday, June 11, 2016 6:43:40 PM
> Subject: Re: [PATCHi next] veth: advertise peer link relationship for both devices
> 
> From: Lance Richardson <lrichard@redhat.com>
> Date: Fri, 10 Jun 2016 12:32:19 -0400
> 
> > Currently, when creating a veth pair, notfications to user
> > space only include link peer for one end of the veth pair:
> >    # ip monitor link &
> >    # ip link add dev vm1 type veth peer name vm2
> >    30: vm2@NONE: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN
> >        link/ether be:e3:b7:0e:14:52 brd ff:ff:ff:ff:ff:ff
> >    31: vm1@vm2: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN
> >        link/ether da:e6:a6:c5:42:54 brd ff:ff:ff:ff:ff:ff
> > 
> > With this change, netlink notifications are sent with complete
> > information for both interfaces of the veth pair:
> > 
> >    # 3: vm2@NONE: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN
> >        link/ether e2:94:54:8a:ac:f5 brd ff:ff:ff:ff:ff:ff
> >    4: vm1@vm2: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN
> >        link/ether b2:05:70:e0:fc:35 brd ff:ff:ff:ff:ff:ff
> >    3: vm2@vm1: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN
> >        link/ether e2:94:54:8a:ac:f5 brd ff:ff:ff:ff:ff:ff
> > 
> > Signed-off-by: Lance Richardson <lrichard@redhat.com>
> 
> I don't know about this.
> 
> First of all, those notifications you get above tell you everything you
> need to know in order to figure out what both ends of the veth pair are.
> 
> In fact, I would say that the vm1@vm2 notification #31 above is the _only_
> one you absolutely need.
> 
> > @@ -466,8 +466,16 @@ static int veth_newlink(struct net *src_net, struct
> > net_device *dev,
> >  
> >  	priv = netdev_priv(peer);
> >  	rcu_assign_pointer(priv->peer, dev);
> > +
> > +	err = rtnl_configure_link(dev, NULL);
> > +	if (err < 0)
> > +		goto err_configure_dev;
> > +
> > +	rtmsg_ifinfo(RTM_NEWLINK, peer, 0, GFP_KERNEL);
> >  	return 0;
> >  
> > +err_configure_dev:
> > +	/* nothing to do */
> >  err_register_dev:
> >  	/* nothing to do */
> >  err_configure_peer:
> 
> If you're registering the peer here explicitly, this means a link configure
> somewhere else is now superfluous.
> 
> I really don't like this change at all, both from a necessity perspective as
> well as from it's implementation.
> 

I'll confess to not being super-happy with it myself, which is why I've
been sitting on this patch for some time now. A hard NAK will help justify
a "will not fix" to the reporter of this issue.

Thanks,

  Lance