On Fri, May 02, 2014 at 09:54:27AM +0300, Alexandru Copot wrote:
> This is only the communication part of the bundles functionality.
> The actual message pre-validation and commits are not implemented.
>
> We also enable OF1.4 for all the tests.
>
> Signed-off-by: Alexandru Copot <alex.miha...@gmail.com>
> Cc: Daniel Baluta <dbal...@ixiacom.com>
> ---
> v6:
> * return error code for invalid inner message size
I'm not too comfortable with the wraparound to UINT_MAX here if
msg->msg->length is less than sizeof(struct ofp_header):
inner_size = ntohs(oh->length) - sizeof *m;
if (ntohs(msg->msg->length) - sizeof *oh > inner_size) {
I guess it is actually OK but it looks suspicious at first glance. So I
rewrote this and a little of the other code.
I started by dropping the inner ofp_header from ofp14_bundle_add_msg, so
that the structure only covers the actual bundle_add message. At that
point ofp14_bundle_add_msg is the same as ofp14_bundle_ctrl_msg, if one
ignores the padding, so I merged them. And then I modified
ofputil_decode_bundle_add() to be, in my opinion, more obviously
correct.
I'm going to apply this to master in a minute. Here's the diff I'm
folding in.
Thank you very much for this contribution, and I hope that you will
continue to work on OpenFlow 1.4 support.
diff --git a/include/openflow/openflow-1.4.h b/include/openflow/openflow-1.4.h
index 6efa3b9..cf83c61 100644
--- a/include/openflow/openflow-1.4.h
+++ b/include/openflow/openflow-1.4.h
@@ -171,31 +171,20 @@ enum ofp14_bundle_flags {
OFPBF_ORDERED = 1 << 1, /* Execute in specified order. */
};
-/* Message structure for ONF_ET_BUNDLE_CONTROL. */
+/* Message structure for OFPT_BUNDLE_CONTROL and OFPT_BUNDLE_ADD_MESSAGE. */
struct ofp14_bundle_ctrl_msg {
ovs_be32 bundle_id; /* Identify the bundle. */
- ovs_be16 type; /* OFPBCT_*. */
+ ovs_be16 type; /* OFPT_BUNDLE_CONTROL: one of OFPBCT_*.
+ * OFPT_BUNDLE_ADD_MESSAGE: not used. */
ovs_be16 flags; /* Bitmap of OFPBF_* flags. */
- /* Bundle Property list. */
- /* struct ofp14_bundle_prop_header properties[0]; */
+ /* Followed by:
+ * - For OFPT_BUNDLE_ADD_MESSAGE only, an encapsulated OpenFlow message,
+ * beginning with an ofp_header whose xid is identical to this message's
+ * outer xid.
+ * - For OFPT_BUNDLE_ADD_MESSAGE only, and only if at least one property is
+ * present, 0 to 7 bytes of padding to align on a 64-bit boundary.
+ * - Zero or more properties (see struct ofp14_bundle_prop_header). */
};
OFP_ASSERT(sizeof(struct ofp14_bundle_ctrl_msg) == 8);
-/* Message structure for OFP_BUNDLE_ADD_MESSAGE.
-* Adding a message in a bundle is done with. */
-struct ofp14_bundle_add_msg {
- ovs_be32 bundle_id; /* Identify the bundle. */
- uint8_t pad[2]; /* Align to 64 bits. */
- ovs_be16 flags; /* Bitmap of ONF_BF_* flags. */
-
- struct ofp_header message; /* Message added to the bundle. */
-
- /* If there is one property or more, 'message' is followed by:
- * - Exactly (message.length + 7)/8*8 - (message.length) (between 0 and 7)
- * bytes of all-zero bytes */
-
- /* Bundle Property list. */
- /* struct ofp14_bundle_prop_header properties[0]; */
-};
-OFP_ASSERT(sizeof(struct ofp14_bundle_add_msg) == 16);
#endif /* openflow/openflow-1.4.h */
diff --git a/lib/ofp-msgs.h b/lib/ofp-msgs.h
index 573b4ce..ded9042 100644
--- a/lib/ofp-msgs.h
+++ b/lib/ofp-msgs.h
@@ -236,7 +236,7 @@ enum ofpraw {
/* OFPT 1.4+ (33): struct ofp14_bundle_ctrl_msg, uint8_t[8][]. */
OFPRAW_OFPT14_BUNDLE_CONTROL,
- /* OFPT 1.4+ (34): struct ofp14_bundle_add_msg, uint8_t[]. */
+ /* OFPT 1.4+ (34): struct ofp14_bundle_ctrl_msg, uint8_t[]. */
OFPRAW_OFPT14_BUNDLE_ADD_MESSAGE,
/* Standard statistics. */
diff --git a/lib/ofp-print.c b/lib/ofp-print.c
index 1dc634b..a2e515d 100644
--- a/lib/ofp-print.c
+++ b/lib/ofp-print.c
@@ -2746,7 +2746,7 @@ ofp_print_bundle_add(struct ds *s, const struct
ofp_header *oh, int verbosity)
ofp_print_bit_names(s, badd.flags, bundle_flags_to_name, ' ');
ds_put_char(s, '\n');
- msg = ofp_to_string(badd.msg, badd.length, verbosity);
+ msg = ofp_to_string(badd.msg, ntohs(badd.msg->length), verbosity);
if (msg) {
ds_put_cstr(s, msg);
}
diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index 804cfb5..d178113 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -7096,23 +7096,22 @@ enum ofperr
ofputil_decode_bundle_add(const struct ofp_header *oh,
struct ofputil_bundle_add_msg *msg)
{
+ const struct ofp14_bundle_ctrl_msg *m;
struct ofpbuf b;
enum ofpraw raw;
- size_t inner_size;
- const struct ofp14_bundle_add_msg *m;
+ size_t inner_len;
ofpbuf_use_const(&b, oh, ntohs(oh->length));
raw = ofpraw_pull_assert(&b);
ovs_assert(raw == OFPRAW_OFPT14_BUNDLE_ADD_MESSAGE);
- m = ofpbuf_l3(&b);
+ m = ofpbuf_pull(&b, sizeof *m);
msg->bundle_id = ntohl(m->bundle_id);
msg->flags = ntohs(m->flags);
- msg->length = ntohs(m->message.length);
- msg->msg = &m->message;
- inner_size = ntohs(oh->length) - sizeof *m;
- if (ntohs(msg->msg->length) - sizeof *oh > inner_size) {
+ msg->msg = ofpbuf_data(&b);
+ inner_len = ntohs(msg->msg->length);
+ if (inner_len < sizeof(struct ofp_header) || inner_len > ofpbuf_size(&b)) {
return OFPERR_OFPBFC_MSG_BAD_LEN;
}
@@ -7124,16 +7123,14 @@ ofputil_encode_bundle_add(enum ofp_version ofp_version,
struct ofputil_bundle_add_msg *msg)
{
struct ofpbuf *request;
- struct ofp14_bundle_add_msg *m;
+ struct ofp14_bundle_ctrl_msg *m;
request = ofpraw_alloc(OFPRAW_OFPT14_BUNDLE_ADD_MESSAGE, ofp_version, 0);
m = ofpbuf_put_zeros(request, sizeof *m);
m->bundle_id = htonl(msg->bundle_id);
m->flags = htons(msg->flags);
-
- ofpbuf_put_zeros(request, msg->length - sizeof(*msg->msg));
- memcpy(&m->message, msg->msg, msg->length);
+ ofpbuf_put(request, msg->msg, ntohs(msg->msg->length));
return request;
}
diff --git a/lib/ofp-util.h b/lib/ofp-util.h
index 3f39b22..782e512 100644
--- a/lib/ofp-util.h
+++ b/lib/ofp-util.h
@@ -1147,7 +1147,6 @@ struct ofputil_bundle_add_msg {
uint32_t bundle_id;
uint16_t flags;
const struct ofp_header *msg;
- uint16_t length;
};
enum ofperr ofputil_decode_bundle_ctrl(const struct ofp_header *,
diff --git a/ofproto/bundles.c b/ofproto/bundles.c
index 8db3667..9904465 100644
--- a/ofproto/bundles.c
+++ b/ofproto/bundles.c
@@ -224,7 +224,6 @@ ofp_bundle_discard(struct ofconn *ofconn, uint32_t id)
enum ofperr
ofp_bundle_add_message(struct ofconn *ofconn, struct ofputil_bundle_add_msg
*badd)
{
- struct ofp_header *msg;
struct hmap *bundles;
struct ofp_bundle *bundle;
struct bundle_message *bmsg;
@@ -245,12 +244,8 @@ ofp_bundle_add_message(struct ofconn *ofconn, struct
ofputil_bundle_add_msg *bad
return OFPERR_OFPBFC_BUNDLE_CLOSED;
}
- msg = xmemdup(badd->msg, badd->length);
-
- bmsg = xmalloc(sizeof(*bmsg));
- bmsg->msg = msg;
-
+ bmsg = xmalloc(sizeof *bmsg);
+ bmsg->msg = xmemdup(badd->msg, ntohs(badd->msg->length));
list_push_back(&bundle->msg_list, &bmsg->node);
-
return 0;
}
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
