On Thu, Dec 27, 2012 at 04:55:57PM +0200, Jarno Rajahalme wrote:
> On Dec 27, 2012, at 7:23 , ext Simon Horman wrote:
> > diff --git a/datapath/actions.c b/datapath/actions.c
> > index f638ffc..3278e37 100644
> > --- a/datapath/actions.c
> > +++ b/datapath/actions.c
> > @@ -49,6 +49,64 @@ static int make_writable(struct sk_buff *skb, int
> > write_len)
> > return pskb_expand_head(skb, 0, 0, GFP_ATOMIC);
> > }
> >
> > +static __be16 get_ethertype(const struct sk_buff *skb)
> > +{
> > + struct ethhdr *hdr = (struct ethhdr *)(skb_cb_mpls_bos(skb) - ETH_HLEN);
> > + return hdr->h_proto;
> > +}
> > +
> > +static void set_ethertype(struct sk_buff *skb, const __be16 ethertype)
> > +{
> > + struct ethhdr *hdr = (struct ethhdr *)(skb_cb_mpls_bos(skb) - ETH_HLEN);
> > + hdr->h_proto = ethertype;
> > +}
> > +
>
> These would fail if skb_cb_mpls_bos() ever returned NULL. According to
> the documented semantics this would happen for every non-MPLS packet.
> However, it seems that OVS_CB(skb)->mpls_bos is actually set for every
> packet, and always contains the length of the Ethernet headers, including
> possible VLAN headers. It would be good to change the comment in
> datapath.h accordingly (see below), and rename the field. See more
> detailed comments below in context.
Thanks, I need to double check but I believe your analysis is correct
and you suggestions seem reasonable. I'll see about updating the patch
as per your suggestions.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
