[ovs-dev] [PATCH v7 5/5] datapath: add layer 3 support to ovs_packet_cmd_execute()

Jesse Gross jesse at nicira.com
Fri Nov 14 21:02:13 UTC 2014


On Fri, Nov 14, 2014 at 3:51 AM, Lorand Jakab <lojakab at cisco.com> wrote:
> diff --git a/datapath/datapath.c b/datapath/datapath.c
> index 3607170..3ecb3cc 100644
> --- a/datapath/datapath.c
> +++ b/datapath/datapath.c
> @@ -570,6 +558,23 @@ static int ovs_packet_cmd_execute(struct sk_buff *skb, struct genl_info *info)
>         if (err)
>                 goto err_flow_free;
>
> +       skb_reset_mac_header(packet);
> +
> +       if (flow->key.phy.is_layer3) {
> +               skb_reset_network_header(packet);

I think the code that was resetting the layer pointers here was mostly
just so that we can access the Ethernet header. Now that it is
happening after the flow extract, I would prefer to centralize all of
this type of code there.

> +       } else {
> +               eth = eth_hdr(packet);
> +
> +               /* Normally, setting the skb 'protocol' field would be handled
> +                * by a call to eth_type_trans(), but it assumes there's a
> +                * sending device, which we may not have.
> +                */
> +               if (ntohs(eth->h_proto) >= ETH_P_802_3_MIN)
> +                       packet->protocol = eth->h_proto;
> +               else
> +                       packet->protocol = htons(ETH_P_802_2);

We already have similar logic in flow extraction, so maybe we can just
use the EtherType set there.

> +       }

Don't we need to set packet->protocol in the is_layer3 case as well?

> diff --git a/datapath/flow.c b/datapath/flow.c
> index b01f7bd..1dd3ac8 100644
> --- a/datapath/flow.c
> +++ b/datapath/flow.c
> +static __be16 ethertype_from_ip_version(struct sk_buff *skb)
> +{
> +       struct iphdr *iphdr = ip_hdr(skb);
> +
> +       if (iphdr->version == 4)
> +               return htons(ETH_P_IP);
> +       else if (iphdr->version == 6)
> +               return htons(ETH_P_IPV6);
> +
> +       return 0;
> +}

I don't really like reaching into the IP header to get the type from
the nibble. In any case, I don't think that it will generalize into
other cases (like MPLS) so it seems better to use the attributes
coming from userspace to figure this out.



More information about the dev mailing list