Until now, composing a fixed-length action with ofpact_put_<NAME>() failed to append any padding required after the action. This commit changes that so that these calls now add padding. This meant that the function ofpact_pad(), which was until now required in various unintuitive places, is no longer required, and removes it.
Variable-length actions still require calling ofpact_update_len() after composition. I don't see a way to avoid that. Signed-off-by: Ben Pfaff <[email protected]> --- lib/learn.c | 1 - lib/learning-switch.c | 1 - lib/ofp-actions.c | 51 ++++++++------------------------------------ lib/ofp-actions.h | 37 +++++++++++++------------------- ofproto/connmgr.c | 1 - ofproto/fail-open.c | 1 - ofproto/ofproto-dpif-xlate.c | 1 - ofproto/ofproto-dpif.c | 1 - 8 files changed, 24 insertions(+), 70 deletions(-) diff --git a/lib/learn.c b/lib/learn.c index 4b2cc97..50627ca 100644 --- a/lib/learn.c +++ b/lib/learn.c @@ -161,7 +161,6 @@ learn_execute(const struct ofpact_learn *learn, const struct flow *flow, break; } } - ofpact_pad(ofpacts); fm->ofpacts = ofpacts->data; fm->ofpacts_len = ofpacts->size; diff --git a/lib/learning-switch.c b/lib/learning-switch.c index a45cf98..ef1a504 100644 --- a/lib/learning-switch.c +++ b/lib/learning-switch.c @@ -650,7 +650,6 @@ process_packet_in(struct lswitch *sw, const struct ofp_header *oh) enqueue->port = out_port; enqueue->queue = queue_id; } - ofpact_pad(&ofpacts); /* Prepare packet_out in case we need one. */ po.buffer_id = pi.buffer_id; diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c index 354c768..921295a 100644 --- a/lib/ofp-actions.c +++ b/lib/ofp-actions.c @@ -444,7 +444,6 @@ ofpacts_pull(struct ofpbuf *ofpacts) { size_t ofs; - ofpact_pad(ofpacts); ofs = ofpacts->size; ofpbuf_pull(ofpacts, ofs); @@ -4376,10 +4375,10 @@ decode_NXAST_RAW_NOTE(const struct nx_action_note *nan, unsigned int length; length = ntohs(nan->len) - offsetof(struct nx_action_note, note); - note = ofpact_put(out, OFPACT_NOTE, - offsetof(struct ofpact_note, data) + length); + note = ofpact_put_NOTE(out); note->length = length; - memcpy(note->data, nan->note, length); + ofpbuf_put(out, nan->note, length); + ofpact_update_len(out, out->header); return 0; } @@ -4930,7 +4929,6 @@ parse_CT(char *arg, struct ofpbuf *ofpacts, const size_t nat_offset = ofpacts_pull(ofpacts); error = parse_NAT(value, ofpacts, usable_protocols); - ofpact_pad(ofpacts); /* Update CT action pointer and length. */ ofpacts->header = ofpbuf_push_uninit(ofpacts, nat_offset); oc = ofpacts->header; @@ -4946,7 +4944,6 @@ parse_CT(char *arg, struct ofpbuf *ofpacts, error = ofpacts_parse_copy(value, ofpacts, &usable_protocols2, false, OFPACT_CT); *usable_protocols &= usable_protocols2; - ofpact_pad(ofpacts); ofpacts->header = ofpbuf_push_uninit(ofpacts, exec_offset); oc = ofpacts->header; } else { @@ -5622,8 +5619,6 @@ ofpacts_decode(const void *actions, size_t actions_len, return error; } } - - ofpact_pad(ofpacts); return 0; } @@ -6297,10 +6292,7 @@ ofpacts_pull_openflow_instructions(struct ofpbuf *openflow, struct ofpact_nest *on; const struct ofp_action_header *actions; size_t actions_len; - size_t start; - - ofpact_pad(ofpacts); - start = ofpacts->size; + size_t start = ofpacts->size; ofpact_put(ofpacts, OFPACT_WRITE_ACTIONS, offsetof(struct ofpact_nest, actions)); get_actions_from_instruction(insts[OVSINST_OFPIT11_WRITE_ACTIONS], @@ -7270,7 +7262,6 @@ ofpact_put(struct ofpbuf *ofpacts, enum ofpact_type type, size_t len) { struct ofpact *ofpact; - ofpact_pad(ofpacts); ofpacts->header = ofpbuf_put_uninit(ofpacts, len); ofpact = ofpacts->header; ofpact_init(ofpact, type, len); @@ -7286,41 +7277,18 @@ ofpact_init(struct ofpact *ofpact, enum ofpact_type type, size_t len) ofpact->len = len; } -/* Updates 'ofpact->len' to the number of bytes in the tail of 'ofpacts' - * starting at 'ofpact'. - * - * This is the correct way to update a variable-length ofpact's length after - * adding the variable-length part of the payload. (See the large comment - * near the end of ofp-actions.h for more information.) */ +/* Finishes composing a variable-length action (begun using + * ofpact_put_<NAME>()), by padding the action to a multiple of OFPACT_ALIGNTO + * bytes and updating its embedded length field. See the large comment near + * the end of ofp-actions.h for more information. */ void ofpact_update_len(struct ofpbuf *ofpacts, struct ofpact *ofpact) { ovs_assert(ofpact == ofpacts->header); ofpact->len = (char *) ofpbuf_tail(ofpacts) - (char *) ofpact; -} - -/* Pads out 'ofpacts' to a multiple of OFPACT_ALIGNTO bytes in length. Each - * ofpact_put_<ENUM>() calls this function automatically beforehand, but the - * client must call this itself after adding the final ofpact to an array of - * them. - * - * (The consequences of failing to call this function are probably not dire. - * OFPACT_FOR_EACH will calculate a pointer beyond the end of the ofpacts, but - * not dereference it. That's undefined behavior, technically, but it will not - * cause a real problem on common systems. Still, it seems better to call - * it.) */ -void -ofpact_pad(struct ofpbuf *ofpacts) -{ - unsigned int pad = PAD_SIZE(ofpacts->size, OFPACT_ALIGNTO); - if (pad) { - ofpbuf_put_zeros(ofpacts, pad); - } + ofpbuf_padto(ofpacts, OFPACT_ALIGN(ofpacts->size)); } - - - static char * OVS_WARN_UNUSED_RESULT ofpact_parse(enum ofpact_type type, char *value, struct ofpbuf *ofpacts, enum ofputil_protocol *usable_protocols) @@ -7424,7 +7392,6 @@ ofpacts_parse__(char *str, struct ofpbuf *ofpacts, } prev_inst = inst; } - ofpact_pad(ofpacts); if (drop && ofpacts->size) { return xstrdup("\"drop\" must not be accompanied by any other action " diff --git a/lib/ofp-actions.h b/lib/ofp-actions.h index 18c7395..0bd33e3 100644 --- a/lib/ofp-actions.h +++ b/lib/ofp-actions.h @@ -896,15 +896,16 @@ void *ofpact_put(struct ofpbuf *, enum ofpact_type, size_t len); * * struct <STRUCT> *ofpact_put_<ENUM>(struct ofpbuf *ofpacts); * - * Appends a new 'ofpact', of length OFPACT_<ENUM>_RAW_SIZE, to 'ofpacts', + * Appends a new 'ofpact', of length OFPACT_<ENUM>_SIZE, to 'ofpacts', * initializes it with ofpact_init_<ENUM>(), and returns it. Also sets * 'ofpacts->header' to the returned action. * * After using this function to add a variable-length action, add the * elements of the flexible array (e.g. with ofpbuf_put()), then use - * ofpact_update_len() to update the length embedded into the action. - * (Keep in mind the need to refresh the structure from 'ofpacts->frame' - * after adding data to 'ofpacts'.) + * ofpact_update_len() to pad the action to a multiple of OFPACT_ALIGNTO + * bytes and update its embedded length field. (Keep in mind the need to + * refresh the structure from 'ofpacts->header' after adding data to + * 'ofpacts'.) * * struct <STRUCT> *ofpact_get_<ENUM>(const struct ofpact *ofpact); * @@ -916,29 +917,22 @@ void *ofpact_put(struct ofpbuf *, enum ofpact_type, size_t len); * void ofpact_init_<ENUM>(struct <STRUCT> *ofpact); * * Initializes the parts of 'ofpact' that identify it as having type - * OFPACT_<ENUM> and length OFPACT_<ENUM>_RAW_SIZE and zeros the rest. - * - * <ENUM>_RAW_SIZE - * - * The size of the action structure. For a fixed-length action, this is - * sizeof(struct <STRUCT>). For a variable-length action, this is the - * offset to the variable-length part. + * OFPACT_<ENUM> and length OFPACT_<ENUM>_SIZE and zeros the rest. * * <ENUM>_SIZE * - * An integer constant, the value of OFPACT_<ENUM>_RAW_SIZE rounded up to a - * multiple of OFPACT_ALIGNTO. + * The size of the action structure. For a fixed-length action, this is + * sizeof(struct <STRUCT>) rounded up to a multiple of OFPACT_ALIGNTO. For + * a variable-length action, this is the offset to the variable-length + * part. */ #define OFPACT(ENUM, STRUCT, MEMBER, NAME) \ BUILD_ASSERT_DECL(offsetof(struct STRUCT, ofpact) == 0); \ \ - enum { OFPACT_##ENUM##_RAW_SIZE \ + enum { OFPACT_##ENUM##_SIZE \ = (offsetof(struct STRUCT, MEMBER) \ ? offsetof(struct STRUCT, MEMBER) \ - : sizeof(struct STRUCT)) }; \ - \ - enum { OFPACT_##ENUM##_SIZE \ - = ROUND_UP(OFPACT_##ENUM##_RAW_SIZE, OFPACT_ALIGNTO) }; \ + : OFPACT_ALIGN(sizeof(struct STRUCT))) }; \ \ static inline struct STRUCT * \ ofpact_get_##ENUM(const struct ofpact *ofpact) \ @@ -951,21 +945,20 @@ void *ofpact_put(struct ofpbuf *, enum ofpact_type, size_t len); ofpact_put_##ENUM(struct ofpbuf *ofpacts) \ { \ return ofpact_put(ofpacts, OFPACT_##ENUM, \ - OFPACT_##ENUM##_RAW_SIZE); \ + OFPACT_##ENUM##_SIZE); \ } \ \ static inline void \ ofpact_init_##ENUM(struct STRUCT *ofpact) \ { \ ofpact_init(&ofpact->ofpact, OFPACT_##ENUM, \ - OFPACT_##ENUM##_RAW_SIZE); \ + OFPACT_##ENUM##_SIZE); \ } OFPACTS #undef OFPACT -/* Functions to use after adding ofpacts to a buffer. */ +/* Call after adding the variable-length part to a variable-length action. */ void ofpact_update_len(struct ofpbuf *, struct ofpact *); -void ofpact_pad(struct ofpbuf *); /* Additional functions for composing ofpacts. */ struct ofpact_set_field *ofpact_put_reg_load(struct ofpbuf *ofpacts); diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c index ef2c06f..9a4da55 100644 --- a/ofproto/connmgr.c +++ b/ofproto/connmgr.c @@ -2062,7 +2062,6 @@ connmgr_flushed(struct connmgr *mgr) ofpbuf_init(&ofpacts, OFPACT_OUTPUT_SIZE); ofpact_put_OUTPUT(&ofpacts)->port = OFPP_NORMAL; - ofpact_pad(&ofpacts); match_init_catchall(&match); ofproto_add_flow(mgr->ofproto, &match, 0, ofpacts.data, diff --git a/ofproto/fail-open.c b/ofproto/fail-open.c index 38e775a..3c9a9ad 100644 --- a/ofproto/fail-open.c +++ b/ofproto/fail-open.c @@ -229,7 +229,6 @@ fail_open_flushed(struct fail_open *fo) * OFPP_NORMAL. */ ofpbuf_init(&ofpacts, OFPACT_OUTPUT_SIZE); ofpact_put_OUTPUT(&ofpacts)->port = OFPP_NORMAL; - ofpact_pad(&ofpacts); match_init_catchall(&match); ofproto_add_flow(fo->ofproto, &match, FAIL_OPEN_PRIORITY, diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index dab64b9..57d877f 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -4100,7 +4100,6 @@ xlate_write_actions(struct xlate_ctx *ctx, const struct ofpact *a) } ofpbuf_put(&ctx->action_set, on->actions, on_len); - ofpact_pad(&ctx->action_set); } static void diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index 6561c65..454d88b 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -1407,7 +1407,6 @@ add_internal_flows(struct ofproto_dpif *ofproto) controller->max_len = UINT16_MAX; controller->controller_id = 0; controller->reason = OFPR_NO_MATCH; - ofpact_pad(&ofpacts); error = add_internal_miss_flow(ofproto, id++, &ofpacts, &ofproto->miss_rule); -- 2.1.3 _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
