[ovs-dev] [PATCH] ofp-util: Rewrite action decoding to improve compiler warnings.
Ethan Jackson
ethan at nicira.com
Sat Aug 6 01:14:34 UTC 2011
Looks good.
Ethan
On Fri, Aug 5, 2011 at 15:47, Ben Pfaff <blp at nicira.com> 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
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
More information about the dev
mailing list