On Fri, Oct 26, 2012 at 05:08:31PM +0900, Isaku Yamahata wrote:
> Some inlined comments in case label == 0. I don't find any dependency on it.
Thanks Yamahata-san,
I agree with both of your comments, I should have fixed these up
when adding tracking of the stack depth (which was also suggested by you).
O will incorporate the changes you suggest below in the next version of
this patch.
> On Thu, Oct 18, 2012 at 06:02:18PM +0900, Simon Horman wrote:
> > diff --git a/datapath/datapath.c b/datapath/datapath.c
> > index a6915fb..e606ae1 100644
> > --- a/datapath/datapath.c
> > +++ b/datapath/datapath.c
> > @@ -74,6 +74,43 @@ int ovs_net_id __read_mostly;
> > int (*ovs_dp_ioctl_hook)(struct net_device *dev, struct ifreq *rq, int
> > cmd);
> > EXPORT_SYMBOL(ovs_dp_ioctl_hook);
> >
> > +void skb_cb_set_mpls_bos(struct sk_buff *skb)
> > +{
> > + struct ethhdr *eth;
> > + int nh_ofs;
> > + __be16 dl_type = 0;
> > +
> > + skb_reset_mac_header(skb);
> > +
> > + eth = eth_hdr(skb);
> > + nh_ofs = sizeof(struct ethhdr);
> > + if (likely(eth->h_proto >= htons(ETH_TYPE_MIN))) {
> > + dl_type = eth->h_proto;
> > +
> > + while (dl_type == htons(ETH_P_8021Q) &&
> > + skb->len >= nh_ofs + sizeof(struct vlan_hdr)) {
> > + struct vlan_hdr *vh = (struct vlan_hdr*)(skb->data +
> > nh_ofs);
> > + dl_type = vh->h_vlan_encapsulated_proto;
> > + nh_ofs += sizeof(struct vlan_hdr);
> > + }
> > +
> > + OVS_CB(skb)->mpls_bos = nh_ofs;
> > + } else {
> > + OVS_CB(skb)->mpls_bos = 0;
> > + }
> > +}
> > +
> > +unsigned char *skb_cb_mpls_bos(const struct sk_buff *skb)
> > +{
> > + return OVS_CB(skb)->mpls_bos ?
> > + skb_mac_header(skb) + OVS_CB(skb)->mpls_bos : 0;
> > +}
> > +
> > +ptrdiff_t skb_cb_mpls_bos_offset(const struct sk_buff *skb)
> > +{
> > + return OVS_CB(skb)->mpls_bos;
> > +}
> > +
> > /**
> > * DOC: Locking:
> > *
> > @@ -663,6 +700,9 @@ static int validate_actions(const struct nlattr *attr,
> > static const u32 action_lens[OVS_ACTION_ATTR_MAX + 1] = {
> > [OVS_ACTION_ATTR_OUTPUT] = sizeof(u32),
> > [OVS_ACTION_ATTR_USERSPACE] = (u32)-1,
> > + [OVS_ACTION_ATTR_PUSH_MPLS] = sizeof(struct
> > ovs_action_push_mpls),
> > + [OVS_ACTION_ATTR_POP_MPLS] = sizeof(__be16),
> > + [OVS_ACTION_ATTR_SET_MPLS] = sizeof(__be32),
> > [OVS_ACTION_ATTR_PUSH_VLAN] = sizeof(struct
> > ovs_action_push_vlan),
> > [OVS_ACTION_ATTR_POP_VLAN] = 0,
> > [OVS_ACTION_ATTR_SET] = (u32)-1,
> > @@ -691,6 +731,24 @@ static int validate_actions(const struct nlattr *attr,
> > return -EINVAL;
> > break;
> >
> > + case OVS_ACTION_ATTR_PUSH_MPLS: {
> > + const struct ovs_action_push_mpls *mpls = nla_data(a);
> > + if (mpls->mpls_ethertype != htons(ETH_P_MPLS_UC) &&
> > + mpls->mpls_ethertype != htons(ETH_P_MPLS_MC))
> > + return -EINVAL;
> > + break;
> > + }
> > +
> > + case OVS_ACTION_ATTR_POP_MPLS:
> > + break;
> > +
> > + case OVS_ACTION_ATTR_SET_MPLS: {
> > + __be32 mpls_label = nla_get_be32(a);
> > + if (mpls_label == htonl(0)) {
> > + return -EINVAL;
> > + }
>
> Is this necessary?
> At least, this is inconsistent with case OVS_ACTION_ATTR_PUSH_MPLS.
>
>
> > + break;
> > + }
> >
> > case OVS_ACTION_ATTR_POP_VLAN:
> > break;
> > @@ -805,6 +863,8 @@ static int ovs_packet_cmd_execute(struct sk_buff *skb,
> > struct genl_info *info)
> > OVS_CB(packet)->flow = flow;
> > packet->priority = flow->key.phy.priority;
> >
> > + skb_cb_set_mpls_bos(packet);
> > +
> > rcu_read_lock();
> > dp = get_dp(sock_net(skb->sk), ovs_header->dp_ifindex);
> > err = -ENODEV;
>
>
> ... snip ...
>
> > diff --git a/datapath/flow.h b/datapath/flow.h
> > index 02c563a..eb3fe49 100644
> > --- a/datapath/flow.h
> > +++ b/datapath/flow.h
> > @@ -53,6 +53,9 @@ struct sw_flow_key {
> > __be16 type; /* Ethernet frame type. */
> > } eth;
> > struct {
> > + __be32 top_label; /* 0 if no MPLS, top label from stack
> > otherwise */
>
> I think that the validity of top_label is guaranteed by ethertype = MPLS.
> So top_label can be 0 as valid label value.
>
>
> > + } mpls;
> > + struct {
> > u8 proto; /* IP protocol or lower 8 bits of ARP
> > opcode. */
> > u8 tos; /* IP ToS. */
> > u8 ttl; /* IP TTL/hop limit. */
> > @@ -126,6 +129,10 @@ struct arp_eth_header {
> > unsigned char ar_tip[4]; /* target IP address */
> > } __packed;
> >
> > +#define ETH_TYPE_MIN 0x600
> > +
> > +#define MPLS_HLEN 4
> > +
> > int ovs_flow_init(void);
> > void ovs_flow_exit(void);
> >
>
> --
> yamahata
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
