On Wed, Oct 26, 2011 at 3:27 PM, Ben Pfaff <b...@nicira.com> wrote:
> diff --git a/datapath/tunnel.c b/datapath/tunnel.c
> index 372d90e..047961f 100644
> --- a/datapath/tunnel.c
> +++ b/datapath/tunnel.c
> +static int tnl_sock_bind(const struct tnl_ops *tnl_ops, struct
> tnl_mutable_config *mutable)
> +{
> +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,26)
> + struct sockaddr_in sin;
> + struct tnl_sock *ts;
> + int err;
> +
> + if (tnl_ops->ipproto != IPPROTO_UDP)
> + return 0;
> +
> + list_for_each_entry (ts, &tnl_socks, list) {
> + if (ts->port == mutable->dst_port) {
> + if (udp_sk(ts->socket->sk)->encap_rcv !=
> tnl_ops->udp_rcv)
> + return -EADDRINUSE;
When I originally proposed this, I thought that we could directly use
the socket lookup to find the tunnel. I now see that it isn't
possible because of the key (and there is no way to know whether the
key is important until after you do the tunnel lookup). Is there any
benefit to binding the socket more specifically to the tunnel using
the remote and local (if there is one) IPs? The main benefit of this
would be to enable finer grained binding of sockets for different
tunnel types. I don't know whether that is more or less confusing
though. It also makes the UDP socket lookup marginally faster.
> @@ -1347,29 +1438,57 @@ static int tnl_set_config(struct nlattr *options,
> const struct tnl_ops *tnl_ops,
[...]
> + if (tnl_ops->ipproto == IPPROTO_UDP) {
> + struct nlattr *src_base = a[OVS_TUNNEL_ATTR_SRC_PORT_BASE];
> + struct nlattr *src_hash = a[OVS_TUNNEL_ATTR_SRC_PORT_HASH];
> + struct nlattr *dst = a[OVS_TUNNEL_ATTR_DST_PORT];
> +
> + if (!src_base || !dst) {
> + err = -EINVAL;
> + goto error;
> + }
I think we should add some validation to check that if ports are
specified then they go with a protocol that has L4 ports. Right now,
I don't think either userspace or kernel will care.
> + mutable->src_port_base = ntohs(nla_get_be16(src_base));
> + mutable->dst_port = nla_get_be16(dst);
Can you add a comment to explain that it is intention that one is byte
swapped and the other is not? When I first saw it, I was sure it was
a bug (although sparse should be good about catching actual bugs).
Otherwise this looks like pretty nice infrastructure.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
