Make dpctl accept an arbitrary number of actions.

This cleanup has been wanted for a while.

As a side effect, this change deletes some dead code pointed out by Chris
Eagle via Fortify: many of the deleted comparisons against act_len were
never true because of the sizes of the objects invovled.

diff --git a/utilities/dpctl.c b/utilities/dpctl.c
index 80f8a46103ecae516f6d27412ff86b9b086b9668..828e33a3fe29c8b8a02ab12c4f5fa0db297d7c0b 100644 (file)
--- a/utilities/dpctl.c
+++ b/utilities/dpctl.c
@@ -599,124 +599,91 @@ str_to_ip(const char *str_, uint32_t *ip)
     return n_wild;
 }
 
+static void *
+put_action(struct ofpbuf *b, size_t size, uint16_t type)
+{
+    struct ofp_action_header *ah = ofpbuf_put_zeros(b, size);
+    ah->type = htons(type);
+    ah->len = htons(size);
+    return ah;
+}
+
+static struct ofp_action_output *
+put_output_action(struct ofpbuf *b, uint16_t port)
+{
+    struct ofp_action_output *oao = put_action(b, sizeof *oao, OFPAT_OUTPUT);
+    oao->port = htons(port);
+    return oao;
+}
+
 static void
-str_to_action(char *str, struct ofp_action_header *actions, 
-        size_t *actions_len) 
+str_to_action(char *str, struct ofpbuf *b)
 {
-    size_t len = *actions_len;
     char *act, *arg;
     char *saveptr = NULL;
-    uint8_t *p = (uint8_t *)actions;
-    
-    memset(actions, 0, len);
-    for (act = strtok_r(str, ", \t\r\n", &saveptr); 
-         (len >= sizeof(struct ofp_action_header)) && act;
+
+    for (act = strtok_r(str, ", \t\r\n", &saveptr); act;
          act = strtok_r(NULL, ", \t\r\n", &saveptr)) 
     {
-        uint16_t port;
-        struct ofp_action_header *ah = (struct ofp_action_header *)p;
-        int act_len = sizeof *ah;
-        port = OFPP_MAX;
-
         /* Arguments are separated by colons */
         arg = strchr(act, ':');
         if (arg) {
             *arg = '\0';
             arg++;
-        } 
+        }
 
         if (!strcasecmp(act, "mod_vlan_vid")) {
-            struct ofp_action_vlan_vid *va = (struct ofp_action_vlan_vid *)ah;
-
-            if (len < sizeof *va) {
-                ofp_fatal(0, "Insufficient room for vlan vid action\n");
-            }
-
-            act_len = sizeof *va;
-            va->type = htons(OFPAT_SET_VLAN_VID);
+            struct ofp_action_vlan_vid *va;
+            va = put_action(b, sizeof *va, OFPAT_SET_VLAN_VID);
             va->vlan_vid = htons(str_to_u32(arg));
         } else if (!strcasecmp(act, "mod_vlan_pcp")) {
-            struct ofp_action_vlan_pcp *va = (struct ofp_action_vlan_pcp *)ah;
-
-            if (len < sizeof *va) {
-                ofp_fatal(0, "Insufficient room for vlan pcp action\n");
-            }
-
-            act_len = sizeof *va;
-            va->type = htons(OFPAT_SET_VLAN_PCP);
+            struct ofp_action_vlan_pcp *va;
+            va = put_action(b, sizeof *va, OFPAT_SET_VLAN_PCP);
             va->vlan_pcp = str_to_u32(arg);
         } else if (!strcasecmp(act, "strip_vlan")) {
+            struct ofp_action_header *ah;
+            ah = put_action(b, sizeof *ah, OFPAT_STRIP_VLAN);
             ah->type = htons(OFPAT_STRIP_VLAN);
         } else if (!strcasecmp(act, "output")) {
-            port = str_to_u32(arg);
+            put_output_action(b, str_to_u32(arg));
 #ifdef SUPPORT_SNAT
         } else if (!strcasecmp(act, "nat")) {
-            struct nx_action_snat *sa = (struct nx_action_snat *)ah;
-
-            if (len < sizeof *sa) {
-                ofp_fatal(0, "Insufficient room for SNAT action\n");
-            }
+            struct nx_action_snat *sa;
 
             if (str_to_u32(arg) > OFPP_MAX) {
                 ofp_fatal(0, "Invalid nat port: %s\n", arg);
             }
 
-            act_len = sizeof *sa;
-            sa->type = htons(OFPAT_VENDOR);
+            sa = put_action(b, sizeof *sa, OFPAT_VENDOR);
             sa->vendor = htonl(NX_VENDOR_ID);
             sa->subtype = htons(NXAST_SNAT);
             sa->port = htons(str_to_u32(arg));
 #endif
         } else if (!strcasecmp(act, "TABLE")) {
-            port = OFPP_TABLE;
+            put_output_action(b, OFPP_TABLE);
         } else if (!strcasecmp(act, "NORMAL")) {
-            port = OFPP_NORMAL;
+            put_output_action(b, OFPP_NORMAL);
         } else if (!strcasecmp(act, "FLOOD")) {
-            port = OFPP_FLOOD;
+            put_output_action(b, OFPP_FLOOD);
         } else if (!strcasecmp(act, "ALL")) {
-            port = OFPP_ALL;
+            put_output_action(b, OFPP_ALL);
         } else if (!strcasecmp(act, "CONTROLLER")) {
-            struct ofp_action_output *ca = (struct ofp_action_output *)ah;
-
-            if (act_len < sizeof *ca) {
-                ofp_fatal(0, "Insufficient room for controller action\n");
-            }
-
-            act_len = sizeof *ca;
-            ca->type = htons(OFPAT_OUTPUT);
-            ca->port = htons(OFPP_CONTROLLER);
+            struct ofp_action_output *oao;
+            oao = put_output_action(b, OFPP_CONTROLLER);
 
             /* Unless a numeric argument is specified, we send the whole
              * packet to the controller. */
             if (arg && (strspn(act, "0123456789") == strlen(act))) {
-               ca->max_len= htons(str_to_u32(arg));
+               oao->max_len = htons(str_to_u32(arg));
             }
         } else if (!strcasecmp(act, "LOCAL")) {
-            port = OFPP_LOCAL;
+            put_output_action(b, OFPP_LOCAL);
         } else if (strspn(act, "0123456789") == strlen(act)) {
-            port = str_to_u32(act);
+            put_output_action(b, str_to_u32(act));
         } else {
             ofp_fatal(0, "Unknown action: %s", act);
         }
-
-        if (port != OFPP_MAX) {
-            struct ofp_action_output *oa = (struct ofp_action_output *)p;
-
-            if (act_len < sizeof *oa) {
-                ofp_fatal(0, "Insufficient room for output action\n");
-            }
-
-            act_len = sizeof *oa;
-            oa->type = htons(OFPAT_OUTPUT);
-            oa->port = htons(port);
-        }
-
-        ah->len = htons(act_len);
-        p += act_len;
-        len -= act_len;
     }
-
-    *actions_len -= len;
 }
 
 struct protocol {
@@ -787,8 +754,7 @@ parse_field(const char *name, const struct field **f_out)
 }
 
 static void
-str_to_flow(char *string, struct ofp_match *match, 
-            struct ofp_action_header *actions, size_t *actions_len, 
+str_to_flow(char *string, struct ofp_match *match, struct ofpbuf *actions,
             uint8_t *table_idx, uint16_t *out_port, uint16_t *priority, 
             uint16_t *idle_timeout, uint16_t *hard_timeout)
 {
@@ -825,7 +791,7 @@ str_to_flow(char *string, struct ofp_match *match,
 
         act_str++;
 
-        str_to_action(act_str, actions, actions_len);
+        str_to_action(act_str, actions);
     }
     memset(match, 0, sizeof *match);
     wildcards = OFPFW_ALL;
@@ -893,7 +859,7 @@ do_dump_flows(const struct settings *s UNUSED, int argc, char *argv[])
     struct ofpbuf *request;
 
     req = alloc_stats_request(sizeof *req, OFPST_FLOW, &request);
-    str_to_flow(argc > 2 ? argv[2] : "", &req->match, NULL, 0, 
+    str_to_flow(argc > 2 ? argv[2] : "", &req->match, NULL,
                 &req->table_id, &out_port, NULL, NULL, NULL);
     memset(&req->pad, 0, sizeof req->pad);
     req->out_port = htons(out_port);
@@ -909,7 +875,7 @@ do_dump_aggregate(const struct settings *s UNUSED, int argc, char *argv[])
     uint16_t out_port;
 
     req = alloc_stats_request(sizeof *req, OFPST_AGGREGATE, &request);
-    str_to_flow(argc > 2 ? argv[2] : "", &req->match, NULL, 0,
+    str_to_flow(argc > 2 ? argv[2] : "", &req->match, NULL,
                 &req->table_id, &out_port, NULL, NULL, NULL);
     memset(&req->pad, 0, sizeof req->pad);
     req->out_port = htons(out_port);
@@ -978,13 +944,10 @@ do_add_flow(const struct settings *s UNUSED, int argc UNUSED, char *argv[])
     struct ofpbuf *buffer;
     struct ofp_flow_mod *ofm;
     uint16_t priority, idle_timeout, hard_timeout;
-    size_t size;
-    size_t actions_len = MAX_ACT_LEN;
 
     /* Parse and send. */
-    size = sizeof *ofm + actions_len;
-    ofm = make_openflow(size, OFPT_FLOW_MOD, &buffer);
-    str_to_flow(argv[2], &ofm->match, &ofm->actions[0], &actions_len, 
+    ofm = make_openflow(sizeof *ofm, OFPT_FLOW_MOD, &buffer);
+    str_to_flow(argv[2], &ofm->match, buffer,
                 NULL, NULL, &priority, &idle_timeout, &hard_timeout);
     ofm->command = htons(OFPFC_ADD);
     ofm->idle_timeout = htons(idle_timeout);
@@ -993,9 +956,6 @@ do_add_flow(const struct settings *s UNUSED, int argc UNUSED, char *argv[])
     ofm->priority = htons(priority);
     ofm->reserved = htonl(0);
 
-    /* xxx Should we use the ofpbuf library? */
-    buffer->size -= MAX_ACT_LEN - actions_len;
-
     open_vconn(argv[1], &vconn);
     send_openflow_buffer(vconn, buffer);
     vconn_close(vconn);
@@ -1018,8 +978,6 @@ do_add_flows(const struct settings *s UNUSED, int argc UNUSED, char *argv[])
         struct ofpbuf *buffer;
         struct ofp_flow_mod *ofm;
         uint16_t priority, idle_timeout, hard_timeout;
-        size_t size;
-        size_t actions_len = MAX_ACT_LEN;
 
         char *comment;
 
@@ -1035,9 +993,8 @@ do_add_flows(const struct settings *s UNUSED, int argc UNUSED, char *argv[])
         }
 
         /* Parse and send. */
-        size = sizeof *ofm + actions_len;
-        ofm = make_openflow(size, OFPT_FLOW_MOD, &buffer);
-        str_to_flow(line, &ofm->match, &ofm->actions[0], &actions_len, 
+        ofm = make_openflow(sizeof *ofm, OFPT_FLOW_MOD, &buffer);
+        str_to_flow(line, &ofm->match, buffer,
                     NULL, NULL, &priority, &idle_timeout, &hard_timeout);
         ofm->command = htons(OFPFC_ADD);
         ofm->idle_timeout = htons(idle_timeout);
@@ -1046,9 +1003,6 @@ do_add_flows(const struct settings *s UNUSED, int argc UNUSED, char *argv[])
         ofm->priority = htons(priority);
         ofm->reserved = htonl(0);
 
-        /* xxx Should we use the ofpbuf library? */
-        buffer->size -= MAX_ACT_LEN - actions_len;
-
         send_openflow_buffer(vconn, buffer);
     }
     vconn_close(vconn);
@@ -1062,13 +1016,10 @@ do_mod_flows(const struct settings *s, int argc UNUSED, char *argv[])
     struct vconn *vconn;
     struct ofpbuf *buffer;
     struct ofp_flow_mod *ofm;
-    size_t size;
-    size_t actions_len = MAX_ACT_LEN;
 
     /* Parse and send. */
-    size = sizeof *ofm + actions_len;
-    ofm = make_openflow(size, OFPT_FLOW_MOD, &buffer);
-    str_to_flow(argv[2], &ofm->match, &ofm->actions[0], &actions_len, 
+    ofm = make_openflow(sizeof *ofm, OFPT_FLOW_MOD, &buffer);
+    str_to_flow(argv[2], &ofm->match, buffer,
                 NULL, NULL, &priority, &idle_timeout, &hard_timeout);
     if (s->strict) {
         ofm->command = htons(OFPFC_MODIFY_STRICT);
@@ -1081,9 +1032,6 @@ do_mod_flows(const struct settings *s, int argc UNUSED, char *argv[])
     ofm->priority = htons(priority);
     ofm->reserved = htonl(0);
 
-    /* xxx Should we use the buffer library? */
-    buffer->size -= MAX_ACT_LEN - actions_len;
-
     open_vconn(argv[1], &vconn);
     send_openflow_buffer(vconn, buffer);
     vconn_close(vconn);
@@ -1096,12 +1044,10 @@ static void do_del_flows(const struct settings *s, int argc, char *argv[])
     uint16_t out_port;
     struct ofpbuf *buffer;
     struct ofp_flow_mod *ofm;
-    size_t size;
 
     /* Parse and send. */
-    size = sizeof *ofm;
-    ofm = make_openflow(size, OFPT_FLOW_MOD, &buffer);
-    str_to_flow(argc > 2 ? argv[2] : "", &ofm->match, NULL, 0, NULL, 
+    ofm = make_openflow(sizeof *ofm, OFPT_FLOW_MOD, &buffer);
+    str_to_flow(argc > 2 ? argv[2] : "", &ofm->match, NULL, NULL, 
                 &out_port, &priority, NULL, NULL);
     if (s->strict) {
         ofm->command = htons(OFPFC_DELETE_STRICT);