Thanks, I pushed this.
On Fri, Aug 05, 2011 at 06:14:34PM -0700, Ethan Jackson wrote: > Looks good. > > Ethan > > On Fri, Aug 5, 2011 at 15:47, Ben Pfaff <[email protected]> wrote: > > The previous implementation of ofputil_decode_action() had two weaknesses. > > First, the action lengths in its tables were written as literal integers > > instead of using "sizeof". ?Second, it used arrays for tables instead of > > switch statements, which meant that GCC didn't warn when someone added a > > new action type but forgot to add an entry to the tables. > > > > This rewrite fixes both of those problems. > > --- > > ?lib/ofp-util.c | ?168 > > ++++++++++++++++++++++++++++++-------------------------- > > ?1 files changed, 91 insertions(+), 77 deletions(-) > > > > diff --git a/lib/ofp-util.c b/lib/ofp-util.c > > index df3377a..d9ebcda 100644 > > --- a/lib/ofp-util.c > > +++ b/lib/ofp-util.c > > @@ -2077,89 +2077,86 @@ validate_actions(const union ofp_action *actions, > > size_t n_actions, > > ? ? return 0; > > ?} > > > > -struct ofputil_ofpat_action { > > - ? ?enum ofputil_action_code code; > > - ? ?unsigned int len; > > -}; > > - > > -static const struct ofputil_ofpat_action ofpat_actions[] = { > > - ? ?{ OFPUTIL_OFPAT_OUTPUT, ? ? ? ?8 }, > > - ? ?{ OFPUTIL_OFPAT_SET_VLAN_VID, ?8 }, > > - ? ?{ OFPUTIL_OFPAT_SET_VLAN_PCP, ?8 }, > > - ? ?{ OFPUTIL_OFPAT_STRIP_VLAN, ? ?8 }, > > - ? ?{ OFPUTIL_OFPAT_SET_DL_SRC, ? 16 }, > > - ? ?{ OFPUTIL_OFPAT_SET_DL_DST, ? 16 }, > > - ? ?{ OFPUTIL_OFPAT_SET_NW_SRC, ? ?8 }, > > - ? ?{ OFPUTIL_OFPAT_SET_NW_DST, ? ?8 }, > > - ? ?{ OFPUTIL_OFPAT_SET_NW_TOS, ? ?8 }, > > - ? ?{ OFPUTIL_OFPAT_SET_TP_SRC, ? ?8 }, > > - ? ?{ OFPUTIL_OFPAT_SET_TP_DST, ? ?8 }, > > - ? ?{ OFPUTIL_OFPAT_ENQUEUE, ? ? ?16 }, > > -}; > > - > > -struct ofputil_nxast_action { > > - ? ?enum ofputil_action_code code; > > +struct ofputil_action { > > + ? ?int code; > > ? ? unsigned int min_len; > > ? ? unsigned int max_len; > > ?}; > > > > -static const struct ofputil_nxast_action nxast_actions[] = { > > - ? ?{ 0, UINT_MAX, UINT_MAX }, /* NXAST_SNAT__OBSOLETE */ > > - ? ?{ OFPUTIL_NXAST_RESUBMIT, ? ? 16, 16 }, > > - ? ?{ OFPUTIL_NXAST_SET_TUNNEL, ? 16, 16 }, > > - ? ?{ 0, UINT_MAX, UINT_MAX }, /* NXAST_DROP_SPOOFED_ARP__OBSOLETE */ > > - ? ?{ OFPUTIL_NXAST_SET_QUEUE, ? ?16, 16 }, > > - ? ?{ OFPUTIL_NXAST_POP_QUEUE, ? ?16, 16 }, > > - ? ?{ OFPUTIL_NXAST_REG_MOVE, ? ? 24, 24 }, > > - ? ?{ OFPUTIL_NXAST_REG_LOAD, ? ? 24, 24 }, > > - ? ?{ OFPUTIL_NXAST_NOTE, ? ? ? ? 16, UINT_MAX }, > > - ? ?{ OFPUTIL_NXAST_SET_TUNNEL64, 24, 24 }, > > - ? ?{ OFPUTIL_NXAST_MULTIPATH, ? ?32, 32 }, > > - ? ?{ OFPUTIL_NXAST_AUTOPATH, ? ? 24, 24 }, > > - ? ?{ OFPUTIL_NXAST_BUNDLE, ? ? ? 32, UINT_MAX }, > > - ? ?{ OFPUTIL_NXAST_BUNDLE_LOAD, ?32, UINT_MAX }, > > -}; > > +static const struct ofputil_action action_bad_type > > + ? ?= { -OFP_MKERR(OFPET_BAD_ACTION, OFPBAC_BAD_TYPE), ? 0, UINT_MAX }; > > +static const struct ofputil_action action_bad_len > > + ? ?= { -OFP_MKERR(OFPET_BAD_ACTION, OFPBAC_BAD_LEN), ? ?0, UINT_MAX }; > > +static const struct ofputil_action action_bad_vendor > > + ? ?= { -OFP_MKERR(OFPET_BAD_ACTION, OFPBAC_BAD_VENDOR), 0, UINT_MAX }; > > > > -static int > > +static const struct ofputil_action * > > ?ofputil_decode_ofpat_action(const union ofp_action *a) > > ?{ > > - ? ?int type = ntohs(a->type); > > - > > - ? ?if (type < ARRAY_SIZE(ofpat_actions)) { > > - ? ? ? ?const struct ofputil_ofpat_action *ooa = &ofpat_actions[type]; > > - ? ? ? ?unsigned int len = ntohs(a->header.len); > > + ? ?enum ofp_action_type type = ntohs(a->type); > > > > - ? ? ? ?return (len == ooa->len > > - ? ? ? ? ? ? ? ?? ooa->code > > - ? ? ? ? ? ? ? ?: -ofp_mkerr(OFPET_BAD_ACTION, OFPBAC_BAD_LEN)); > > - ? ?} else { > > - ? ? ? ?return -ofp_mkerr(OFPET_BAD_ACTION, OFPBAC_BAD_TYPE); > > + ? ?switch (type) { > > +#define OFPAT_ACTION(ENUM, TYPE) ? ? ? ? ? ? ? ? ? ? ? ? ? ?\ > > + ? ? ? ?case ENUM: { ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\ > > + ? ? ? ? ? ?static const struct ofputil_action action = { ? \ > > + ? ? ? ? ? ? ? ?OFPUTIL_##ENUM, sizeof(TYPE), sizeof(TYPE) ?\ > > + ? ? ? ? ? ?}; ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\ > > + ? ? ? ? ? ?return &action; ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \ > > + ? ? ? ?} > > + ? ? ? ?OFPAT_ACTION(OFPAT_OUTPUT, ? ? ? struct ofp_action_output); > > + ? ? ? ?OFPAT_ACTION(OFPAT_SET_VLAN_VID, struct ofp_action_vlan_vid); > > + ? ? ? ?OFPAT_ACTION(OFPAT_SET_VLAN_PCP, struct ofp_action_vlan_pcp); > > + ? ? ? ?OFPAT_ACTION(OFPAT_STRIP_VLAN, ? struct ofp_action_header); > > + ? ? ? ?OFPAT_ACTION(OFPAT_SET_DL_SRC, ? struct ofp_action_dl_addr); > > + ? ? ? ?OFPAT_ACTION(OFPAT_SET_DL_DST, ? struct ofp_action_dl_addr); > > + ? ? ? ?OFPAT_ACTION(OFPAT_SET_NW_SRC, ? struct ofp_action_nw_addr); > > + ? ? ? ?OFPAT_ACTION(OFPAT_SET_NW_DST, ? struct ofp_action_nw_addr); > > + ? ? ? ?OFPAT_ACTION(OFPAT_SET_NW_TOS, ? struct ofp_action_nw_tos); > > + ? ? ? ?OFPAT_ACTION(OFPAT_SET_TP_SRC, ? struct ofp_action_tp_port); > > + ? ? ? ?OFPAT_ACTION(OFPAT_SET_TP_DST, ? struct ofp_action_tp_port); > > + ? ? ? ?OFPAT_ACTION(OFPAT_ENQUEUE, ? ? ?struct ofp_action_enqueue); > > +#undef OFPAT_ACTION > > + > > + ? ?case OFPAT_VENDOR: > > + ? ?default: > > + ? ? ? ?return &action_bad_type; > > ? ? } > > ?} > > > > -static int > > +static const struct ofputil_action * > > ?ofputil_decode_nxast_action(const union ofp_action *a) > > ?{ > > - ? ?unsigned int len = ntohs(a->header.len); > > - > > - ? ?if (len < sizeof(struct nx_action_header)) { > > - ? ? ? ?return -ofp_mkerr(OFPET_BAD_ACTION, OFPBAC_BAD_LEN); > > - ? ?} else { > > - ? ? ? ?const struct nx_action_header *nah = (const void *) a; > > - ? ? ? ?int subtype = ntohs(nah->subtype); > > - > > - ? ? ? ?if (subtype <= ARRAY_SIZE(nxast_actions)) { > > - ? ? ? ? ? ?const struct ofputil_nxast_action *ona = > > &nxast_actions[subtype]; > > - ? ? ? ? ? ?if (len >= ona->min_len && len <= ona->max_len) { > > - ? ? ? ? ? ? ? ?return ona->code; > > - ? ? ? ? ? ?} else if (ona->min_len == UINT_MAX) { > > - ? ? ? ? ? ? ? ?return -ofp_mkerr(OFPET_BAD_ACTION, OFPBAC_BAD_TYPE); > > - ? ? ? ? ? ?} else { > > - ? ? ? ? ? ? ? ?return -ofp_mkerr(OFPET_BAD_ACTION, OFPBAC_BAD_LEN); > > - ? ? ? ? ? ?} > > - ? ? ? ?} else { > > - ? ? ? ? ? ?return -ofp_mkerr(OFPET_BAD_ACTION, OFPBAC_BAD_TYPE); > > + ? ?const struct nx_action_header *nah = (const struct nx_action_header *) > > a; > > + ? ?enum nx_action_subtype subtype = ntohs(nah->subtype); > > + > > + ? ?switch (subtype) { > > +#define NXAST_ACTION(ENUM, TYPE, EXTENSIBLE) ? ? ? ? ? ? ? ?\ > > + ? ? ? ?case ENUM: { ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\ > > + ? ? ? ? ? ?static const struct ofputil_action action = { ? \ > > + ? ? ? ? ? ? ? ?OFPUTIL_##ENUM, ? ? ? ? ? ? ? ? ? ? ? ? ? ? \ > > + ? ? ? ? ? ? ? ?sizeof(TYPE), ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \ > > + ? ? ? ? ? ? ? ?EXTENSIBLE ? UINT_MAX : sizeof(TYPE) ? ? ? ?\ > > + ? ? ? ? ? ?}; ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\ > > + ? ? ? ? ? ?return &action; ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \ > > ? ? ? ? } > > + ? ? ? ?NXAST_ACTION(NXAST_RESUBMIT, ? ? struct nx_action_resubmit, ? ? > > false); > > + ? ? ? ?NXAST_ACTION(NXAST_SET_TUNNEL, ? struct nx_action_set_tunnel, ? > > false); > > + ? ? ? ?NXAST_ACTION(NXAST_SET_QUEUE, ? ?struct nx_action_set_queue, ? > > ?false); > > + ? ? ? ?NXAST_ACTION(NXAST_POP_QUEUE, ? ?struct nx_action_pop_queue, ? > > ?false); > > + ? ? ? ?NXAST_ACTION(NXAST_REG_MOVE, ? ? struct nx_action_reg_move, ? ? > > false); > > + ? ? ? ?NXAST_ACTION(NXAST_REG_LOAD, ? ? struct nx_action_reg_load, ? ? > > false); > > + ? ? ? ?NXAST_ACTION(NXAST_NOTE, ? ? ? ? struct nx_action_note, ? ? ? ? > > true); > > + ? ? ? ?NXAST_ACTION(NXAST_SET_TUNNEL64, struct nx_action_set_tunnel64, > > false); > > + ? ? ? ?NXAST_ACTION(NXAST_MULTIPATH, ? ?struct nx_action_multipath, ? > > ?false); > > + ? ? ? ?NXAST_ACTION(NXAST_AUTOPATH, ? ? struct nx_action_autopath, ? ? > > false); > > + ? ? ? ?NXAST_ACTION(NXAST_BUNDLE, ? ? ? struct nx_action_bundle, ? ? ? > > true); > > + ? ? ? ?NXAST_ACTION(NXAST_BUNDLE_LOAD, ?struct nx_action_bundle, ? ? ? > > true); > > +#undef NXAST_ACTION > > + > > + ? ?case NXAST_SNAT__OBSOLETE: > > + ? ?case NXAST_DROP_SPOOFED_ARP__OBSOLETE: > > + ? ?default: > > + ? ? ? ?return &action_bad_type; > > ? ? } > > ?} > > > > @@ -2176,13 +2173,28 @@ ofputil_decode_nxast_action(const union ofp_action > > *a) > > ?int > > ?ofputil_decode_action(const union ofp_action *a) > > ?{ > > + ? ?const struct ofputil_action *action; > > + ? ?uint16_t len = ntohs(a->header.len); > > + > > ? ? if (a->type != htons(OFPAT_VENDOR)) { > > - ? ? ? ?return ofputil_decode_ofpat_action(a); > > - ? ?} else if (a->vendor.vendor == htonl(NX_VENDOR_ID)) { > > - ? ? ? ?return ofputil_decode_nxast_action(a); > > + ? ? ? ?action = ofputil_decode_ofpat_action(a); > > ? ? } else { > > - ? ? ? ?return -ofp_mkerr(OFPET_BAD_ACTION, OFPBAC_BAD_VENDOR); > > + ? ? ? ?switch (ntohl(a->vendor.vendor)) { > > + ? ? ? ?case NX_VENDOR_ID: > > + ? ? ? ? ? ?if (len < sizeof(struct nx_action_header)) { > > + ? ? ? ? ? ? ? ?return -ofp_mkerr(OFPET_BAD_ACTION, OFPBAC_BAD_LEN); > > + ? ? ? ? ? ?} > > + ? ? ? ? ? ?action = ofputil_decode_nxast_action(a); > > + ? ? ? ? ? ?break; > > + ? ? ? ?default: > > + ? ? ? ? ? ?action = &action_bad_vendor; > > + ? ? ? ? ? ?break; > > + ? ? ? ?} > > ? ? } > > + > > + ? ?return (len >= action->min_len && len <= action->max_len > > + ? ? ? ? ? ?? action->code > > + ? ? ? ? ? ?: -ofp_mkerr(OFPET_BAD_ACTION, OFPBAC_BAD_LEN)); > > ?} > > > > ?/* Parses 'a' and returns its type as an OFPUTIL_OFPAT_* or OFPUTIL_NXAST_* > > @@ -2192,13 +2204,15 @@ ofputil_decode_action(const union ofp_action *a) > > ?enum ofputil_action_code > > ?ofputil_decode_action_unsafe(const union ofp_action *a) > > ?{ > > + ? ?const struct ofputil_action *action; > > + > > ? ? if (a->type != htons(OFPAT_VENDOR)) { > > - ? ? ? ?return ofpat_actions[ntohs(a->type)].code; > > + ? ? ? ?action = ofputil_decode_ofpat_action(a); > > ? ? } else { > > - ? ? ? ?const struct nx_action_header *nah = (const void *) a; > > - > > - ? ? ? ?return nxast_actions[ntohs(nah->subtype)].code; > > + ? ? ? ?action = ofputil_decode_nxast_action(a); > > ? ? } > > + > > + ? ?return action->code; > > ?} > > > > ?/* Returns true if 'action' outputs to 'port', false otherwise. */ > > -- > > 1.7.4.4 > > > > _______________________________________________ > > dev mailing list > > [email protected] > > http://openvswitch.org/mailman/listinfo/dev > > _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
