On Wed, Jul 13, 2011 at 10:45 AM, pravin shelar <pshe...@nicira.com> wrote:
> following patch changes OVS VLAN actions from MODIFY and
> STRIP to more generic PUSH and POP. As this patch
> replaces MODIFY with PUSH semantic, action mapping done in userpace is
> fixed accordingly.
Can you cleanup the commit message a little as it will be part of the
permanent record? For example, the subject shouldn't have a version
number (that can go in brackets with PATCH if you want),
capitalization, consistent line wrapping, maybe a little more
explanation of why this is being done. Also, we require that any
kernel code have a signed-off-by line to avoid any issues there with
upstreaming.
For your first commit, you can also add yourself to the AUTHORS file.
> diff --git a/datapath/actions.c b/datapath/actions.c
> index ed61039..a91eca5 100644
> --- a/datapath/actions.c
> +++ b/datapath/actions.c
> +static int __pop_vlan_tci(struct sk_buff *skb, __be16 *current_tci)
> {
> struct ethhdr *eh;
> + struct vlan_ethhdr *veth;
> int err;
>
> - if (vlan_tx_tag_present(skb)) {
> - vlan_set_tci(skb, 0);
> - return 0;
> - }
> -
> - if (unlikely(skb->protocol != htons(ETH_P_8021Q) ||
> - skb->len < VLAN_ETH_HLEN))
> - return 0;
>
Extra blank line here.
> +static int pop_vlan_tci(struct sk_buff *skb)
> {
> - if (!vlan_tx_tag_present(skb) && skb->protocol == htons(ETH_P_8021Q))
> {
> - int err;
> + __be16 tci;
> + int err;
>
> - if (unlikely(skb->len < VLAN_ETH_HLEN))
> + if (vlan_tx_tag_present(skb)) {
> + vlan_set_tci(skb, 0);
> + } else {
> + if (unlikely(skb->protocol != htons(ETH_P_8021Q) ||
> + skb->len < VLAN_ETH_HLEN))
> return 0;
>
> - err = strip_vlan(skb);
> - if (unlikely(err))
> + err = __pop_vlan_tci(skb, &tci);
> + if (err)
Can you annotate the various error paths with unlikely()?
> @@ -270,12 +293,14 @@ static int do_execute_actions(struct datapath *dp,
> struct sk_buff *skb,
> OVS_CB(skb)->tun_id = nla_get_be64(a);
> break;
>
> - case ODP_ACTION_ATTR_SET_DL_TCI:
> - err = modify_vlan_tci(skb, nla_get_be16(a));
> + case ODP_ACTION_ATTR_PUSH_VLAN_TCI:
> + err = push_vlan_tci(skb, nla_get_be16(a));
> + if (unlikely(err)) /* skb already freed */
> + return err;
> break;
>
> - case ODP_ACTION_ATTR_STRIP_VLAN:
> - err = strip_vlan(skb);
> + case ODP_ACTION_ATTR_POP_VLAN_TCI:
> + err = pop_vlan_tci(skb);
I would probably drop the TCI portion of these names, since we are
actually pushing the entire tag.
> diff --git a/include/openvswitch/datapath-protocol.h
> b/include/openvswitch/datapath-protocol.h
> index e7708ef..d972a25 100644
> --- a/include/openvswitch/datapath-protocol.h
> +++ b/include/openvswitch/datapath-protocol.h
> @@ -410,8 +410,8 @@ enum odp_action_type {
> ODP_ACTION_ATTR_UNSPEC,
> ODP_ACTION_ATTR_OUTPUT, /* Output to switch port. */
> ODP_ACTION_ATTR_CONTROLLER, /* Send copy to controller. */
> - ODP_ACTION_ATTR_SET_DL_TCI, /* Set the 802.1q TCI value. */
> - ODP_ACTION_ATTR_STRIP_VLAN, /* Strip the 802.1q header. */
> + ODP_ACTION_ATTR_PUSH_VLAN_TCI, /* push 802.1q header with new TCI
> value. */
> + ODP_ACTION_ATTR_POP_VLAN_TCI, /* pop the 802.1q header. */
Can you use capitalization to match the other comments here?
> diff --git a/lib/odp-util.c b/lib/odp-util.c
> index d7a3118..32fb722 100644
> --- a/lib/odp-util.c
> +++ b/lib/odp-util.c
> @@ -106,13 +106,13 @@ format_odp_action(struct ds *ds, const struct nlattr *a)
> ds_put_format(ds, "set_tunnel(%#"PRIx64")",
> ntohll(nl_attr_get_be64(a)));
> break;
> - case ODP_ACTION_ATTR_SET_DL_TCI:
> - ds_put_format(ds, "set_tci(vid=%"PRIu16",pcp=%d)",
> + case ODP_ACTION_ATTR_PUSH_VLAN_TCI:
> + ds_put_format(ds, "push_tci(vid=%"PRIu16",pcp=%d)",
I would print "push_vlan" here for consistency with "pop_vlan" below.
> diff --git a/lib/packets.h b/lib/packets.h
> index 8e13a25..b9cfa1d 100644
> --- a/lib/packets.h
> +++ b/lib/packets.h
> @@ -131,7 +131,7 @@ void compose_benign_packet(struct ofpbuf *, const char
> *tag,
> uint16_t snap_type,
> const uint8_t eth_src[ETH_ADDR_LEN]);
>
> -void eth_set_vlan_tci(struct ofpbuf *, ovs_be16 tci);
> +void dp_netdev_push_vlan(struct ofpbuf *, ovs_be16 tci);
You should keep the same prefix as before in the name since this is
generic code, not only used by dp_netdev.
> @@ -3461,13 +3463,16 @@ compose_actions(struct action_xlate_ctx *ctx,
> uint16_t vlan,
> }
> if (dst->vlan != cur_vlan) {
> if (dst->vlan == OFP_VLAN_NONE) {
> - nl_msg_put_flag(ctx->odp_actions,
> ODP_ACTION_ATTR_STRIP_VLAN);
> + nl_msg_put_flag(ctx->odp_actions,
> ODP_ACTION_ATTR_POP_VLAN_TCI);
> } else {
> ovs_be16 tci;
> +
> + if (cur_vlan & htons(VLAN_CFI))
> + nl_msg_put_flag(ctx->odp_actions,
> ODP_ACTION_ATTR_POP_VLAN_TCI);
I think in this context you need to check whether cur_vlan is not
OFP_VLAN_NONE (this is the OpenFlow convention for vlans instead of
the Linux/Open vSwitch convention).
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
