From 109ee2810ddf3c26d4c32e64208ca2033965d6db Mon Sep 17 00:00:00 2001
From: Ben Pfaff <blp@nicira.com>
Date: Wed, 5 Oct 2011 09:04:50 -0700
Subject: [PATCH] dpif-netdev: Simplify code by removing
 dpif_netdev_validate_actions().

dpif_netdev_validate_actions() existed for three reasons.  First, it checked
that the actions were well-formed and valid.  This isn't really necessary,
because the actions are built internally by ofproto-dpif and will always be
well-formed.  (If not, that's a bug in ofproto-dpif.)  Second, it checks
whether the actions will modify (mutate) the data in the packet and reports
that to the caller, which can use it to optimize what it does.  However,
the only caller that used this was dpif_netdev_execute(), which is not a
fast-path (if dpif-netdev can be said to have a fast path at all).

Third, dpif_netdev_validate_actions() rejects certain actions that
dpif-netdev does not implement: OVS_ACTION_ATTR_SET_TUNNEL,
OVS_ACTION_ATTR_SET_PRIORITY, and OVS_ACTION_ATTR_POP_PRIORITY.  However,
this doesn't really seem necessary to me.  First, dpif-netdev can't support
tunnels in any case, so OVS_ACTION_ATTR_SET_TUNNEL shouldn't come up.
Second, the priority actions just aren't important enough to worry about;
they only affect QoS, which isn't really important with dpif-netdev since
it's going to be slow anyway.

So this commit just drops dpif_netdev_validate_actions() entirely.
---
 lib/dpif-netdev.c | 96 +++--------------------------------------------
 1 file changed, 6 insertions(+), 90 deletions(-)

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index af62bf0c..43f6c43c 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -691,78 +691,10 @@ dpif_netdev_flow_get(const struct dpif *dpif,
     return 0;
 }
 
-static int
-dpif_netdev_validate_actions(const struct nlattr *actions,
-                             size_t actions_len, bool *mutates)
-{
-    const struct nlattr *a;
-    unsigned int left;
-
-    *mutates = false;
-    NL_ATTR_FOR_EACH (a, left, actions, actions_len) {
-        uint16_t type = nl_attr_type(a);
-        int len = odp_action_len(type);
-
-        if (len != nl_attr_get_size(a)) {
-            return EINVAL;
-        }
-
-        switch (type) {
-        case OVS_ACTION_ATTR_OUTPUT:
-            if (nl_attr_get_u32(a) >= MAX_PORTS) {
-                return EINVAL;
-            }
-            break;
-
-        case OVS_ACTION_ATTR_USERSPACE:
-            break;
-
-        case OVS_ACTION_ATTR_PUSH_VLAN:
-            *mutates = true;
-            if (nl_attr_get_be16(a) & htons(VLAN_CFI)) {
-                return EINVAL;
-            }
-            break;
-
-        case OVS_ACTION_ATTR_SET_NW_TOS:
-            *mutates = true;
-            if (nl_attr_get_u8(a) & IP_ECN_MASK) {
-                return EINVAL;
-            }
-            break;
-
-        case OVS_ACTION_ATTR_POP_VLAN:
-        case OVS_ACTION_ATTR_SET_DL_SRC:
-        case OVS_ACTION_ATTR_SET_DL_DST:
-        case OVS_ACTION_ATTR_SET_NW_SRC:
-        case OVS_ACTION_ATTR_SET_NW_DST:
-        case OVS_ACTION_ATTR_SET_TP_SRC:
-        case OVS_ACTION_ATTR_SET_TP_DST:
-            *mutates = true;
-            break;
-
-        case OVS_ACTION_ATTR_SET_TUNNEL:
-        case OVS_ACTION_ATTR_SET_PRIORITY:
-        case OVS_ACTION_ATTR_POP_PRIORITY:
-        default:
-            return EOPNOTSUPP;
-        }
-    }
-    return 0;
-}
-
 static int
 set_flow_actions(struct dp_netdev_flow *flow,
                  const struct nlattr *actions, size_t actions_len)
 {
-    bool mutates;
-    int error;
-
-    error = dpif_netdev_validate_actions(actions, actions_len, &mutates);
-    if (error) {
-        return error;
-    }
-
     flow->actions = xrealloc(flow->actions, actions_len);
     flow->actions_len = actions_len;
     memcpy(flow->actions, actions, actions_len);
@@ -956,7 +888,6 @@ dpif_netdev_execute(struct dpif *dpif,
 {
     struct dp_netdev *dp = get_dp_netdev(dpif);
     struct ofpbuf copy;
-    bool mutates;
     struct flow key;
     int error;
 
@@ -964,24 +895,10 @@ dpif_netdev_execute(struct dpif *dpif,
         return EINVAL;
     }
 
-    error = dpif_netdev_validate_actions(actions, actions_len, &mutates);
-    if (error) {
-        return error;
-    }
-
-    if (mutates) {
-        /* We need a deep copy of 'packet' since we're going to modify its
-         * data. */
-        ofpbuf_init(&copy, DP_NETDEV_HEADROOM + packet->size);
-        ofpbuf_reserve(&copy, DP_NETDEV_HEADROOM);
-        ofpbuf_put(&copy, packet->data, packet->size);
-    } else {
-        /* We still need a shallow copy of 'packet', even though we won't
-         * modify its data, because flow_extract() modifies packet->l2, etc.
-         * We could probably get away with modifying those but it's more polite
-         * if we don't. */
-        copy = *packet;
-    }
+    /* Make a deep copy of 'packet', because we might modify its data. */
+    ofpbuf_init(&copy, DP_NETDEV_HEADROOM + packet->size);
+    ofpbuf_reserve(&copy, DP_NETDEV_HEADROOM);
+    ofpbuf_put(&copy, packet->data, packet->size);
 
     flow_extract(&copy, 0, -1, &key);
     error = dpif_netdev_flow_from_nlattrs(key_attrs, key_len, &key);
@@ -989,9 +906,8 @@ dpif_netdev_execute(struct dpif *dpif,
         error = dp_netdev_execute_actions(dp, &copy, &key,
                                           actions, actions_len);
     }
-    if (mutates) {
-        ofpbuf_uninit(&copy);
-    }
+
+    ofpbuf_uninit(&copy);
     return error;
 }
 
-- 
2.30.2