From 12f0bf6d978199796f5877c54aec49423bfc9d0e Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Fri, 9 Jan 2009 13:28:12 -0800 Subject: [PATCH] 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. --- utilities/dpctl.c | 156 +++++++++++++++------------------------------- 1 file changed, 51 insertions(+), 105 deletions(-) diff --git a/utilities/dpctl.c b/utilities/dpctl.c index 80f8a461..828e33a3 100644 --- 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); -- 2.30.2