| Message ID | 1366175423-27310-5-git-send-email-amwang@redhat.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, 2013-04-17 at 13:10 +0800, Cong Wang wrote: > From: Cong Wang <amwang@redhat.com> > > As David suggested, we should support ll addr, which requires > scope id. David, any comments on this patch? I am not sure it is correct. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Cong Wang <amwang@redhat.com> wrote on 04/17/2013 01:10:21 AM: > diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c > index 43ed40f..531c5e2 100644 > --- a/drivers/net/vxlan.c > +++ b/drivers/net/vxlan.c > @@ -92,9 +92,10 @@ struct vxlan_addr { > struct sockaddr_in6 sin6; > struct sockaddr sa; > } u; > -#define va_sin u.sin.sin_addr.s_addr > -#define va_sin6 u.sin6.sin6_addr > -#define va_sa u.sa.sa_family > +#define va_sin u.sin.sin_addr.s_addr > +#define va_sin6 u.sin6.sin6_addr > +#define va_scope_id u.sin6.sin6_scope_id > +#define va_sa u.sa.sa_family > }; As I commented before, you're obscuring the types here, which makes it less readable. "va_sin6" ought to be a sockaddr_in6, not a sin6_addr, and the scope id ought to then be "va_sin6.sin6_scope_id", without any other #define necessary for it. > @@ -1708,6 +1712,17 @@ static int vxlan_newlink(struct net *net, > struct net_device *dev, > /* update header length based on lower device */ > dev->hard_header_len = lowerdev->hard_header_len + > VXLAN_HEADROOM; > +#if IS_ENABLED(CONFIG_IPV6) > + dst->remote_ip.va_scope_id = ipv6_iface_scope_id > (&dst->remote_ip.va_sin6, > + dst->remote_ifindex); > + if (ipv6_addr_type(&dst->remote_ip.va_sin6) & IPV6_ADDR_LINKLOCAL) { > + struct vxlan_net *vn = net_generic(net, vxlan_net_id); > + struct sock *sk = vn->sock->sk; > + > + sk->sk_bound_dev_if = dst->remote_ip.va_scope_id; > + } > +#endif > + > } The socket is not used for transmits, which is where the sin6_scope_id is relevant-- we need this in vxlan_xmit_one() for the route lookup. It also permanently binds to an interface, which I believe prevents VXLAN traffic from other interfaces on receive, and this socket is an INADDR_ANY-bound socket for all VXLAN devices on the host. So, we shouldn't be doing anything with sk_bound_dev_if, but LL addrs need to use the the sin6_scope_id in the route lookup for transmissions. It should be saved in remote_ifindex per destination and stored in flowi6_oif for route lookup, as the v4 code does now for fdb entries, and LL v6 destinations for fdb entries ought to be done the same way. So, no, I don't think this patch is correct or complete. +-DLS -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2013-04-19 at 07:14 -0400, David Stevens wrote: > Cong Wang <amwang@redhat.com> wrote on 04/17/2013 01:10:21 AM: > > > diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c > > index 43ed40f..531c5e2 100644 > > --- a/drivers/net/vxlan.c > > +++ b/drivers/net/vxlan.c > > @@ -92,9 +92,10 @@ struct vxlan_addr { > > struct sockaddr_in6 sin6; > > struct sockaddr sa; > > } u; > > -#define va_sin u.sin.sin_addr.s_addr > > -#define va_sin6 u.sin6.sin6_addr > > -#define va_sa u.sa.sa_family > > +#define va_sin u.sin.sin_addr.s_addr > > +#define va_sin6 u.sin6.sin6_addr > > +#define va_scope_id u.sin6.sin6_scope_id > > +#define va_sa u.sa.sa_family > > }; > > As I commented before, you're obscuring the types here, > which makes it less readable. "va_sin6" ought to be a sockaddr_in6, > not a sin6_addr, and the scope id ought to then be > "va_sin6.sin6_scope_id", > without any other #define necessary for it. OK, so #define va_sin6 u.sin6? > > So, no, I don't think this patch is correct or complete. > Then let's defer it, I need more time to make it working. I will discard the scope_id patch for now, and make a correct one later. Thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2013-04-19 at 07:14 -0400, David Stevens wrote: > > As I commented before, you're obscuring the types here, > which makes it less readable. "va_sin6" ought to be a sockaddr_in6, > not a sin6_addr, and the scope id ought to then be > "va_sin6.sin6_scope_id", > without any other #define necessary for it. So finally you want something like: vxlan->default_dst.remote_ip.va_sin.sin_addr.s_addr and vxlan->default_dst.remote_ip.va_sin6.sin6_addr Enjoy the readable code! :) Again, I don't like this at all from the beginning! I just want to make David happy. v5 is on the way... For the record, please don't blame me if you think the code is ugly. :) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Cong Wang <amwang@redhat.com> writes: > On Fri, 2013-04-19 at 07:14 -0400, David Stevens wrote: >> >> As I commented before, you're obscuring the types here, >> which makes it less readable. "va_sin6" ought to be a sockaddr_in6, >> not a sin6_addr, and the scope id ought to then be >> "va_sin6.sin6_scope_id", >> without any other #define necessary for it. > > So finally you want something like: > > vxlan->default_dst.remote_ip.va_sin.sin_addr.s_addr > > and > > vxlan->default_dst.remote_ip.va_sin6.sin6_addr > > Enjoy the readable code! :) There's nothing preventing you from using local variables and/or functions to shorten those, without obscuring things on the way. Readability depends on more than just "short". Obviously correct type is more important. But you can do both. Most kernel code does. It's not hard. struct vxlan_addr { union { struct sockaddr_in sin; struct sockaddr_in6 sin6; struct sockaddr sa; } u; #define va_sin u.sin #define va_sin6 u.sin6 #define va_sa u.sa }; struct vxlan_addr *rip = &vxlan->default_dst.remote_ip; rip->va_sin.sin_addr.s_addr rip->va_sin6.sin6_addr etc. Bjørn -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, 2013-04-21 at 16:46 +0200, Bjørn Mork wrote: > struct vxlan_addr *rip = &vxlan->default_dst.remote_ip; > > rip->va_sin.sin_addr.s_addr > rip->va_sin6.sin6_addr I don't see any difference between this with: vxlan->default_dst.remote_ip.va_sin.sin_addr.s_addr vxlan->default_dst.remote_ip.va_sin6.sin6_addr except making two lines shorter... People still have to go into 5 levels to get the final field. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Cong Wang <amwang@redhat.com> writes: > On Sun, 2013-04-21 at 16:46 +0200, Bjørn Mork wrote: >> struct vxlan_addr *rip = &vxlan->default_dst.remote_ip; >> >> rip->va_sin.sin_addr.s_addr >> rip->va_sin6.sin6_addr > > I don't see any difference between this with: > > vxlan->default_dst.remote_ip.va_sin.sin_addr.s_addr > vxlan->default_dst.remote_ip.va_sin6.sin6_addr > > except making two lines shorter... People still have to go into 5 levels > to get the final field. Yes, of course. That's how many levels there is in this data structure. They still have to go into 5 levels when you define away some of those levels. Only difference is that they then have to _remember_ that one of the labels actually is a macro hiding 3 levels... You are obscuring the levels, not removing them. That's the problem. Noone objects to removing levels if you can do that by moving fields around. But if you can't, then don't try to hide it. Bjørn -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c index 43ed40f..531c5e2 100644 --- a/drivers/net/vxlan.c +++ b/drivers/net/vxlan.c @@ -92,9 +92,10 @@ struct vxlan_addr { struct sockaddr_in6 sin6; struct sockaddr sa; } u; -#define va_sin u.sin.sin_addr.s_addr -#define va_sin6 u.sin6.sin6_addr -#define va_sa u.sa.sa_family +#define va_sin u.sin.sin_addr.s_addr +#define va_sin6 u.sin6.sin6_addr +#define va_scope_id u.sin6.sin6_scope_id +#define va_sa u.sa.sa_family }; struct vxlan_rdst { @@ -1674,6 +1675,9 @@ static int vxlan_newlink(struct net *net, struct net_device *dev, nla_memcpy(&dst->remote_ip.va_sin6, data[IFLA_VXLAN_REMOTE6], sizeof(struct in6_addr)); dst->remote_ip.va_sa = AF_INET6; + if (__ipv6_addr_needs_scope_id(__ipv6_addr_type(&dst->remote_ip.va_sin6)) + && !data[IFLA_VXLAN_LINK]) + return -EINVAL; #else return -EPFNOSUPPORT; #endif @@ -1708,6 +1712,17 @@ static int vxlan_newlink(struct net *net, struct net_device *dev, /* update header length based on lower device */ dev->hard_header_len = lowerdev->hard_header_len + VXLAN_HEADROOM; +#if IS_ENABLED(CONFIG_IPV6) + dst->remote_ip.va_scope_id = ipv6_iface_scope_id(&dst->remote_ip.va_sin6, + dst->remote_ifindex); + if (ipv6_addr_type(&dst->remote_ip.va_sin6) & IPV6_ADDR_LINKLOCAL) { + struct vxlan_net *vn = net_generic(net, vxlan_net_id); + struct sock *sk = vn->sock->sk; + + sk->sk_bound_dev_if = dst->remote_ip.va_scope_id; + } +#endif + } if (data[IFLA_VXLAN_TOS])