[ovs-dev] [PATCH 11/15] ofproto-dpif-xlate: Add IPv6 ND support for XC_TNL_ARP

[Ben Pfaff]

On Wed, Nov 11, 2015 at 11:57:04AM -0200, Thadeu Lima de Souza Cascardo wrote:
> On Tue, Nov 10, 2015 at 04:03:13PM -0800, Ben Pfaff wrote:
> > On Thu, Oct 22, 2015 at 03:29:04PM -0200, Thadeu Lima de Souza Cascardo wrote:
> > > Use IPv4-mapped addresses and use either tnl_arp_lookup or tnl_nd_lookup.
> > > 
> > > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo at redhat.com>
> > 
> > ...
> > 
> > > @@ -5348,8 +5349,13 @@ xlate_push_stats(struct xlate_cache *xcache,
> > >              break;
> > >          case XC_TNL_ARP:
> > >              /* Lookup arp to avoid arp timeout. */
> > > -            tnl_arp_lookup(entry->u.tnl_arp_cache.br_name,
> > > -                           entry->u.tnl_arp_cache.d_ip, &dmac);
> > > +            d_ip = in6_addr_get_mapped_ipv4(&entry->u.tnl_arp_cache.d_ipv6);
> > > +            if (d_ip) {
> > > +                tnl_arp_lookup(entry->u.tnl_arp_cache.br_name, d_ip, &dmac);
> > > +            } else {
> > > +                tnl_nd_lookup(entry->u.tnl_arp_cache.br_name,
> > > +                               &entry->u.tnl_arp_cache.d_ipv6, &dmac);
> > > +            }
> > 
> > This code seems a little silly to me, because it is going to some
> > trouble to distinguish IPv4 from IPv6 and pick the correct
> > tnl_*_lookup() function, and either function it picks is going to
> > convert that right back to do the lookup.  I think it would be more
> > sensible to export tnl_arp_lookup__() so that the double conversion
> > isn't needed.
> > 
> > Thanks,
> > 
> > Ben.
> 
> Urgh. Well, I could just use tnl_nd_lookup here with an extra comment
> about that. Exporting tnl_arp_lookup__ would require exporting struct
> tnl_arp_entry.  But I see your point. The only bad point for me in
> using tnl_nd_lookup directly would be the naming. But since we already
> have an IPv6 address in our hands, why not just add a comment assuring
> this will work on IPv4-mapped addresses as well?

If the naming is an issue, you could invent a more generic name,
e.g. "tnl_lookup_mac_to_ip_binding".  But I don't think it's a big deal.

Thanks,

Ben.