Re: [PATCH v3 net-next 1/2] UDP tunnel encapsulation module for tunnelling different protocols like MPLS,IP,NSH etc.

[Martin Varghese <martinvarghesenokia@gmail.com>]

On Fri, Nov 29, 2019 at 01:18:36PM -0500, Willem de Bruijn wrote:
> On Thu, Nov 28, 2019 at 11:25 AM Martin Varghese
> <martinvarghesenokia@gmail.com> wrote:
> >
> > On Mon, Nov 18, 2019 at 12:23:09PM -0500, Willem de Bruijn wrote:
> > > On Sat, Nov 16, 2019 at 12:45 AM Martin Varghese
> > > <martinvarghesenokia@gmail.com> wrote:
> > > >
> > > > From: Martin Varghese <martin.varghese@nokia.com>
> > > >
> > > > The Bareudp tunnel module provides a generic L3 encapsulation
> > > > tunnelling module for tunnelling different protocols like MPLS,
> > > > IP,NSH etc inside a UDP tunnel.
> > > >
> > > > Signed-off-by: Martin Varghese <martin.varghese@nokia.com>
> 
> > > > +static int bareudp_fill_metadata_dst(struct net_device *dev,
> > > > +                                    struct sk_buff *skb)
> > > > +{
> > > > +       struct ip_tunnel_info *info = skb_tunnel_info(skb);
> > > > +       struct bareudp_dev *bareudp = netdev_priv(dev);
> > > > +       bool use_cache = ip_tunnel_dst_cache_usable(skb, info);
> > > > +
> > > > +       if (ip_tunnel_info_af(info) == AF_INET) {
> > > > +               struct rtable *rt;
> > > > +               struct flowi4 fl4;
> > > > +
> > > > +               rt = iptunnel_get_v4_rt(skb, dev, bareudp->net, &fl4, info,
> > > > +                                       use_cache);
> > > > +               if (IS_ERR(rt))
> > > > +                       return PTR_ERR(rt);
> > > > +
> > > > +               ip_rt_put(rt);
> > > > +               info->key.u.ipv4.src = fl4.saddr;
> > > > +#if IS_ENABLED(CONFIG_IPV6)
> > > > +       } else if (ip_tunnel_info_af(info) == AF_INET6) {
> > > > +               struct dst_entry *dst;
> > > > +               struct flowi6 fl6;
> > > > +               struct bareudp_sock *bs6 = rcu_dereference(bareudp->sock);
> > > > +
> > > > +               dst = ip6tunnel_get_dst(skb, dev, bareudp->net, bs6->sock, &fl6,
> > > > +                                       info, use_cache);
> > > > +               if (IS_ERR(dst))
> > > > +                       return PTR_ERR(dst);
> > > > +
> > > > +               dst_release(dst);
> > > > +               info->key.u.ipv6.src = fl6.saddr;
> > > > +#endif
> > > > +       } else {
> > > > +               return -EINVAL;
> > > > +       }
> > > > +
> > > > +       info->key.tp_src = udp_flow_src_port(bareudp->net, skb,
> > > > +                                            bareudp->sport_min,
> > > > +                                            USHRT_MAX, true);
> > > > +       info->key.tp_dst = bareudp->conf.port;
> > > > +       return 0;
> > > > +}
> > >
> > > This can probably all be deduplicated with geneve_fill_metadata_dst
> > > once both use iptunnel_get_v4_rt.
> > >
> >
> > Do you have any preference of file to keep the common function
> 
> Perhaps net/ipv4/udp_tunnel.c

I was trying this change and i found i dont have a lot of generic code here.

Populating L4 ports is function of the protocol implementation
and it is differnt for geneve and for bareudp

The one thing we could do is to write a generic API to derive the 
tunnel src address.

But since it is a very small piece i dont see much value add by making it 
generic and i prefer to keep the way as it is now

But i am open for both the option. What you think ?