All of this looks pretty good to me, I just have a few last things
that I noticed:
On Tue, Oct 20, 2015 at 11:11 PM, John W. Linville
<linvi...@tuxdriver.com> wrote:
> diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
> index 8f5c02eed47d..217b472ab9e7 100644
> --- a/drivers/net/geneve.c
> +++ b/drivers/net/geneve.c
> /* Pseudo network device */
> struct geneve_dev {
> struct hlist_node hlist; /* vni hash table */
> struct net *net; /* netns for packet i/o */
> struct net_device *dev; /* netdev for geneve tunnel */
> - struct geneve_sock *sock; /* socket used for geneve tunnel */
> + struct geneve_sock *sock4; /* IPv4 socket used for geneve tunnel
> */
> + struct geneve_sock *sock6; /* IPv6 socket used for geneve tunnel
> */
It might be worth wrapping sock6 in #if IS_ENABLED(CONFIG_IPV6). Not
because I'm trying to save space but because we make initializing it
conditional on this, so it would catch if somebody tries to access it
uninitialized.
> +static int geneve_open(struct net_device *dev)
> +{
> + struct geneve_dev *geneve = netdev_priv(dev);
> + bool ipv6 = geneve->remote.sa.sa_family == AF_INET6;
> + bool metadata = !!geneve->collect_md;
A small thing but geneve->collect_md is also a bool, so I guess we
probably don't need the !!.
> +#if IS_ENABLED(CONFIG_IPV6)
> +static int geneve6_build_skb(struct dst_entry *dst, struct sk_buff *skb,
> + __be16 tun_flags, u8 vni[3], u8 opt_len, u8 *opt,
> + bool csum, bool xnet)
> +{
[...]
> + skb_scrub_packet(skb, xnet);
I realized that I wasn't really all that clear with my previous
comment here. I was just asking if we should also use
skb_scrub_packet() in the IPv4 to keep things consistent. I agree that
the rt/dst differences makes sharing code a bit difficult in the
general sense.
> +static netdev_tx_t geneve6_xmit_skb(struct sk_buff *skb, struct net_device
> *dev,
> + struct ip_tunnel_info *info)
[...]
> + if (geneve->collect_md) {
> + if (unlikely(info && !(info->mode & IP_TUNNEL_INFO_TX))) {
> + netdev_dbg(dev, "no tunnel metadata\n");
> + goto tx_error;
It seems like this should also be checking for !info here - that's
perhaps the main cause of not having any tunnel metadata. This is the
same with IPv4 though too.
> @@ -870,14 +1139,29 @@ static int geneve_newlink(struct net *net, struct
> net_device *dev,
> - if (!data[IFLA_GENEVE_ID] || !data[IFLA_GENEVE_REMOTE])
> + if (!data[IFLA_GENEVE_ID] ||
> + (data[IFLA_GENEVE_REMOTE] && data[IFLA_GENEVE_REMOTE6]) ||
> + (!data[IFLA_GENEVE_REMOTE] && !data[IFLA_GENEVE_REMOTE6]))
> return -EINVAL;
I think this will conflict with/revert my change in -net. Obviously,
the conflict will need to be resolved at some point, but it's probably
just best to remove the whole block here so the resolution is obvious.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
