| Message ID | 1445534948-10538-12-git-send-email-cascardo@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
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@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.
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@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? I will send something like that. Thanks. Cascardo.
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@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.
On Wed, Nov 11, 2015 at 08:29:32AM -0800, Ben Pfaff wrote: > 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@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. maybe tnl_neigh_lookup() ? fbl
On Thu, Nov 12, 2015 at 02:38:48AM -0200, Flavio Leitner wrote: > On Wed, Nov 11, 2015 at 08:29:32AM -0800, Ben Pfaff wrote: > > 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@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. > > maybe tnl_neigh_lookup() ? > > fbl > I thought of that, but since we are giving an IPv6 address to tnl_nd_lookup, we could claim that tnl_neigh_lookup works with both IPv4 and IPv6 addresses. But I would implement that using IPv4-mapped addresses anyway. And tnl_nd_lookup already supports that. If there were two different databases, that could make sense, but we have only one for both mappings. I think commenting that is good enough. Cascardo.
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 07c1197..65e1e27 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -423,7 +423,7 @@ struct xc_entry { } group; struct { char br_name[IFNAMSIZ]; - ovs_be32 d_ip; + struct in6_addr d_ipv6; } tnl_arp_cache; } u; }; @@ -2774,7 +2774,7 @@ build_tunnel_send(struct xlate_ctx *ctx, const struct xport *xport, entry = xlate_cache_add_entry(ctx->xin->xcache, XC_TNL_ARP); ovs_strlcpy(entry->u.tnl_arp_cache.br_name, out_dev->xbridge->name, sizeof entry->u.tnl_arp_cache.br_name); - entry->u.tnl_arp_cache.d_ip = d_ip; + in6_addr_set_mapped_ipv4(&entry->u.tnl_arp_cache.d_ipv6, d_ip); } xlate_report(ctx, "tunneling from "ETH_ADDR_FMT" "IP_FMT @@ -5305,6 +5305,7 @@ xlate_push_stats(struct xlate_cache *xcache, struct xc_entry *entry; struct ofpbuf entries = xcache->entries; struct eth_addr dmac; + ovs_be32 d_ip; if (!stats->n_packets) { return; @@ -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); + } break; default: OVS_NOT_REACHED();
Use IPv4-mapped addresses and use either tnl_arp_lookup or tnl_nd_lookup. Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@redhat.com> --- ofproto/ofproto-dpif-xlate.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)