Hi Harald,
> On 7/13/17, 12:35 AM, "Harald Welte" <lafo...@gnumonks.org> wrote:
>
> > +static int gtp_dev_open(struct net_device *dev)
> > +{
> > + struct gtp_dev *gtp = netdev_priv(dev);
> > + struct net *net = gtp->net;
> > + struct socket *sock1u;
> > + struct socket *sock0;
> > + struct udp_tunnel_sock_cfg tunnel_cfg;
> > + struct udp_port_cfg udp_conf;
> > + int err;
> > +
> > + memset(&udp_conf, 0, sizeof(udp_conf));
> > +
> > + udp_conf.family = AF_INET;
> > + udp_conf.local_ip.s_addr = htonl(INADDR_ANY);
> > + udp_conf.local_udp_port = htons(GTP1U_PORT);
> > +
> > + err = udp_sock_create(gtp->net, &udp_conf, &sock1u);
> > + if (err < 0)
> > + return err;
> > +
> > + udp_conf.local_udp_port = htons(GTP0_PORT);
> > + err = udp_sock_create(gtp->net, &udp_conf, &sock0);
> > + if (err < 0)
> > + return err;
>
> you're unconditionally binding to both GTP0 and GTP1 UDP ports. This is
> done selectively based on netlink attributes in the existing "normal"
> non-OVS kernel code, i.e. the control is left to the user.
>
> Is this function is only called/used in the context of OVS? If so,
> since you explicitly implement only GTPv1, why bind to GTPv0 port?
>
I had doubts on how this flow-based GTPv1 code path should fit in, which is why
the GTPv0 and the GTPv1 code pieces are mixed in my changes. Should I
explicitly claim that the flow-based change is for GTPv1 only?
> > + setup_udp_tunnel_sock(net, sock1u, &tunnel_cfg);
>
> even here, you're only setting up the v1 and not v0.
>
same reason as above.
> > + /* Assume largest header, ie. GTPv0. */
> > + dev->needed_headroom = LL_MAX_HEADER +
> > + sizeof(struct iphdr) +
> > + sizeof(struct udphdr) +
> > + sizeof(struct gtp0_header);
>
> ... and here you're using headroom for a GTPv0 header, despite (I think)
> only supporting GTPv1 from this configuration?
Yes, only GTPv1 is supported.
>
> > + err = gtp_hashtable_new(gtp, GTP_PDP_HASHSIZE); // JO: when to free??
>
> I think that question about when to free needs to be resolved before any
> merge. Did you check that it persists even after the device is
> closed/removed?
I didn't investigate it. What do you mean by persist?
Thanks
-Jiannan
