mir/ovs
2
0
Fork 0
mirror of https://github.com/openvswitch/ovs synced 2025-09-04 08:15:25 +00:00
Code Issues Releases Wiki Activity

ofp-actions: Make composing actions harder to screw up.

Browse Source 
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 <blp@ovn.org>
Acked-by: Russell Bryant <russell@ovn.org>
This commit is contained in:
Ben Pfaff
2015-12-23 13:23:48 -08:00
parent ad61c98d7a
commit 2bd318dec2
8 changed files with 24 additions and 72 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
lib/learn.c
View File

@@ -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;

1
lib/learning-switch.c
View File

@@ -642,7 +642,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;

53
lib/ofp-actions.c
View File

@@ -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],
@@ -6334,8 +6326,6 @@ ofpacts_pull_openflow_instructions(struct ofpbuf *openflow,
ogt->table_id = oigt->table_id;
}
ofpact_pad(ofpacts);
error = ofpacts_verify(ofpacts->data, ofpacts->size,
(1u << N_OVS_INSTRUCTIONS) - 1, 0);
exit:
@@ -7272,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);
@@ -7288,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)
@@ -7426,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 "

37
lib/ofp-actions.h
View File

@@ -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);

1
ofproto/connmgr.c
View File

@@ -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,

1
ofproto/fail-open.c
View File

@@ -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,

1
ofproto/ofproto-dpif-xlate.c
View File

@@ -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

1
ofproto/ofproto-dpif.c
View File

@@ -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);