It's pretty easy to forget to update the pointer to an ofpact when
finishing it. This commit forces the caller to pass a pointer-to-pointer
instead, and uses that to automatically update the pointer. There still
could be cases that retain other pointers into the ofpbuf, but I imagine
that this is harder to misuse.
Suggested-by: Joe Stringer <j...@ovn.org>
Signed-off-by: Ben Pfaff <b...@ovn.org>
---
lib/bundle.c | 2 +-
lib/learn.c | 2 +-
lib/ofp-actions.c | 34 +++++++++++++++++++---------------
lib/ofp-actions.h | 22 +++++++++++++++++++---
ofproto/ofproto-dpif.c | 2 +-
ovn/lib/actions.c | 2 +-
6 files changed, 42 insertions(+), 22 deletions(-)
diff --git a/lib/bundle.c b/lib/bundle.c
index 0626cb2..4509d25 100644
--- a/lib/bundle.c
+++ b/lib/bundle.c
@@ -179,7 +179,7 @@ bundle_parse__(const char *s, char **save_ptr,
bundle = ofpacts->header;
bundle->n_slaves++;
}
- bundle = ofpact_finish(ofpacts, &bundle->ofpact);
+ ofpact_finish_BUNDLE(ofpacts, &bundle);
bundle->basis = atoi(basis);
if (!strcasecmp(fields, "eth_src")) {
diff --git a/lib/learn.c b/lib/learn.c
index effb827..ca0cb6b 100644
--- a/lib/learn.c
+++ b/lib/learn.c
@@ -380,7 +380,7 @@ learn_parse__(char *orig, char *arg, struct ofpbuf *ofpacts)
}
}
}
- ofpact_finish(ofpacts, &learn->ofpact);
+ ofpact_finish_LEARN(ofpacts, &learn);
return NULL;
}
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index aac4ff0..106ebc8 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -668,7 +668,7 @@ decode_NXAST_RAW_CONTROLLER(const struct
nx_action_controller *nac,
oc->max_len = ntohs(nac->max_len);
oc->controller_id = ntohs(nac->controller_id);
oc->reason = nac->reason;
- ofpact_finish(out, &oc->ofpact);
+ ofpact_finish_CONTROLLER(out, &oc);
return 0;
}
@@ -737,7 +737,7 @@ decode_NXAST_RAW_CONTROLLER2(const struct
nx_action_controller2 *nac2,
}
}
- ofpact_finish(out, &oc->ofpact);
+ ofpact_finish_CONTROLLER(out, &oc);
return 0;
}
@@ -852,7 +852,7 @@ parse_CONTROLLER(char *arg, struct ofpbuf *ofpacts,
controller = ofpacts->header;
controller->userdata_len = userdata_len;
}
- ofpact_finish(ofpacts, &controller->ofpact);
+ ofpact_finish_CONTROLLER(ofpacts, &controller);
}
return NULL;
@@ -1262,7 +1262,7 @@ decode_bundle(bool load, const struct nx_action_bundle
*nab,
bundle = ofpacts->header;
}
- bundle = ofpact_finish(ofpacts, &bundle->ofpact);
+ ofpact_finish_BUNDLE(ofpacts, &bundle);
if (!error) {
error = bundle_check(bundle, OFPP_MAX, NULL);
}
@@ -3096,7 +3096,7 @@ decode_OFPAT_RAW_DEC_NW_TTL(struct ofpbuf *out)
ids->n_controllers = 1;
ofpbuf_put(out, &id, sizeof id);
ids = out->header;
- ofpact_finish(out, &ids->ofpact);
+ ofpact_finish_DEC_TTL(out, &ids);
return error;
}
@@ -3133,7 +3133,7 @@ decode_NXAST_RAW_DEC_TTL_CNT_IDS(const struct
nx_action_cnt_ids *nac_ids,
ids = out->header;
}
- ofpact_finish(out, &ids->ofpact);
+ ofpact_finish_DEC_TTL(out, &ids);
return 0;
}
@@ -3172,7 +3172,7 @@ parse_noargs_dec_ttl(struct ofpbuf *ofpacts)
ofpbuf_put(ofpacts, &id, sizeof id);
ids = ofpacts->header;
ids->n_controllers++;
- ofpact_finish(ofpacts, &ids->ofpact);
+ ofpact_finish_DEC_TTL(ofpacts, &ids);
}
static char * OVS_WARN_UNUSED_RESULT
@@ -3199,7 +3199,7 @@ parse_DEC_TTL(char *arg, struct ofpbuf *ofpacts,
return xstrdup("dec_ttl_cnt_ids: expected at least one controller "
"id.");
}
- ofpact_finish(ofpacts, &ids->ofpact);
+ ofpact_finish_DEC_TTL(ofpacts, &ids);
}
return NULL;
}
@@ -4228,7 +4228,7 @@ decode_NXAST_RAW_LEARN(const struct nx_action_learn *nal,
get_subfield(spec->n_bits, &p, &spec->dst);
}
}
- ofpact_finish(ofpacts, &learn->ofpact);
+ ofpact_finish_LEARN(ofpacts, &learn);
if (!is_all_zeros(p, (char *) end - (char *) p)) {
return OFPERR_OFPBAC_BAD_ARGUMENT;
@@ -4554,7 +4554,7 @@ decode_NXAST_RAW_NOTE(const struct nx_action_note *nan,
note = ofpact_put_NOTE(out);
note->length = length;
ofpbuf_put(out, nan->note, length);
- ofpact_finish(out, out->header);
+ ofpact_finish_NOTE(out, ¬e);
return 0;
}
@@ -4586,7 +4586,7 @@ parse_NOTE(const char *arg, struct ofpbuf *ofpacts,
struct ofpact_note *note = ofpbuf_at_assert(ofpacts, start_ofs,
sizeof *note);
note->length = ofpacts->size - (start_ofs + sizeof *note);
- ofpact_finish(ofpacts, ¬e->ofpact);
+ ofpact_finish_NOTE(ofpacts, ¬e);
return NULL;
}
@@ -4994,7 +4994,7 @@ decode_NXAST_RAW_CT(const struct nx_action_conntrack *nac,
conntrack = ofpbuf_push_uninit(out, sizeof(*conntrack));
out->header = &conntrack->ofpact;
- conntrack = ofpact_finish(out, &conntrack->ofpact);
+ ofpact_finish_CT(out, &conntrack);
if (conntrack->ofpact.len > sizeof(*conntrack)
&& !(conntrack->flags & NX_CT_F_COMMIT)) {
@@ -5115,7 +5115,7 @@ parse_CT(char *arg, struct ofpbuf *ofpacts,
}
}
- ofpact_finish(ofpacts, &oc->ofpact);
+ ofpact_finish_CT(ofpacts, &oc);
ofpbuf_push_uninit(ofpacts, ct_offset);
return error;
}
@@ -7419,6 +7419,7 @@ ofpacts_format(const struct ofpact *ofpacts, size_t
ofpacts_len,
/* Internal use by helpers. */
+/* Implementation of ofpact_put_<ENUM>(). */
void *
ofpact_put(struct ofpbuf *ofpacts, enum ofpact_type type, size_t len)
{
@@ -7430,6 +7431,7 @@ ofpact_put(struct ofpbuf *ofpacts, enum ofpact_type type,
size_t len)
return ofpact;
}
+/* Implementation of ofpact_init_<ENUM>(). */
void
ofpact_init(struct ofpact *ofpact, enum ofpact_type type, size_t len)
{
@@ -7438,8 +7440,10 @@ ofpact_init(struct ofpact *ofpact, enum ofpact_type
type, size_t len)
ofpact->raw = -1;
ofpact->len = len;
}
-
-/* Finishes composing a variable-length action (begun using
+
+/* Implementation of ofpact_finish_<ENUM>().
+ *
+ * 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.
diff --git a/lib/ofp-actions.h b/lib/ofp-actions.h
index 4bd8854..7aa87f1 100644
--- a/lib/ofp-actions.h
+++ b/lib/ofp-actions.h
@@ -903,6 +903,7 @@ const char *ofpact_name(enum ofpact_type);
/* Internal use by the helpers below. */
void ofpact_init(struct ofpact *, enum ofpact_type, size_t len);
void *ofpact_put(struct ofpbuf *, enum ofpact_type, size_t len);
+void *ofpact_finish(struct ofpbuf *, struct ofpact *);
/* For each OFPACT_<ENUM> with a corresponding struct <STRUCT>, this defines
* the following commonly useful functions:
@@ -924,6 +925,16 @@ void *ofpact_put(struct ofpbuf *, enum ofpact_type, size_t
len);
* Returns 'ofpact' cast to "struct <STRUCT> *". 'ofpact->type' must be
* OFPACT_<ENUM>.
*
+ * void ofpact_finish_<ENUM>(struct ofpbuf *ofpacts, struct <STRUCT> **ap);
+ *
+ * Finishes composing variable-length action '*ap' (begun using
+ * ofpact_put_<NAME>() on 'ofpacts'), by padding the action to a multiple
+ * of OFPACT_ALIGNTO bytes and updating its embedded length field.
+ *
+ * May reallocate 'ofpacts', and so as a convenience automatically updates
+ * '*ap' to point to the new location. If the caller has other pointers
+ * within 'ap' or 'ofpacts', it needs to update them manually.
+ *
* as well as the following more rarely useful definitions:
*
* void ofpact_init_<ENUM>(struct <STRUCT> *ofpact);
@@ -965,13 +976,18 @@ void *ofpact_put(struct ofpbuf *, enum ofpact_type,
size_t len);
{ \
ofpact_init(&ofpact->ofpact, OFPACT_##ENUM, \
OFPACT_##ENUM##_SIZE); \
+ } \
+ \
+ static inline void \
+ ofpact_finish_##ENUM(struct ofpbuf *ofpbuf, struct STRUCT **ofpactp) \
+ { \
+ struct ofpact *ofpact = &(*ofpactp)->ofpact; \
+ ovs_assert(ofpact->type == OFPACT_##ENUM); \
+ *ofpactp = ofpact_finish(ofpbuf, ofpact); \
}
OFPACTS
#undef OFPACT
-/* Call after adding the variable-length part to a variable-length action. */
-void *ofpact_finish(struct ofpbuf *, struct ofpact *);
-
/* Additional functions for composing ofpacts. */
struct ofpact_set_field *ofpact_put_reg_load(struct ofpbuf *ofpacts);
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 530c49a..6be7b0a 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -1411,7 +1411,7 @@ add_internal_flows(struct ofproto_dpif *ofproto)
controller->max_len = UINT16_MAX;
controller->controller_id = 0;
controller->reason = OFPR_IMPLICIT_MISS;
- controller = ofpact_finish(&ofpacts, &controller->ofpact);
+ ofpact_finish_CONTROLLER(&ofpacts, &controller);
error = add_internal_miss_flow(ofproto, id++, &ofpacts,
&ofproto->miss_rule);
diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
index a17b5a7..88822b5 100644
--- a/ovn/lib/actions.c
+++ b/ovn/lib/actions.c
@@ -205,7 +205,7 @@ finish_controller_op(struct ofpbuf *ofpacts, size_t ofs)
struct ofpact_controller *oc = ofpbuf_at_assert(ofpacts, ofs, sizeof *oc);
ofpacts->header = oc;
oc->userdata_len = ofpacts->size - (ofs + sizeof *oc);
- ofpact_finish(ofpacts, &oc->ofpact);
+ ofpact_finish_CONTROLLER(ofpacts, &oc);
}
static void
--
2.1.3
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
