On Wed, Sep 09, 2015 at 05:33:42PM +0530, niti1...@gmail.com wrote:
> From: Niti Rohilla <niti.rohi...@tcs.com>
>
> This patch adds support for Openflow1.4 Group & meter change notification
> messages. In a multi controller environment, when a controller modifies the
> state of group and meter table, the request that successfully modifies this
> state is forwarded to other controllers. Other controllers are informed with
> the OFPT_REQUESTFORWARD message. Request forwarding is enabled on a per
> controller channel basis using the Set Asynchronous Configuration Message.
>
> Signed-off-by: Niti Rohilla <niti.rohi...@tcs.com>
> ---
> Difference between v7->v8:
> - Memory leak issue in ofputil_re_encode_requestforward() has been resolved.
> - Modified the test case so that ofputil_re_encode_requestforward() get
> excercised in the testsuite to re-encode the request.
> - Rebased with the latest master.
After I looked at this for a while, I realized that there was more
decoding than necessary. It shouldn't be necessary to decode a message
once to execute it, then decode it again to reencode it inside
ofputil_re_encode_requestforward(). That even introduces a possibility
for failure the second time (though it shouldn't happen).
I decided that it's better, therefore, to make ofputil_requestforward
include the decoded form of the inner message instead of the raw form,
like this:
struct ofputil_requestforward {
ovs_be32 xid;
enum ofp14_requestforward_reason reason;
union {
/* reason == OFPRFR_METER_MOD. */
struct ofputil_meter_mod *meter_mod;
/* reason == OFPRFR_GROUP_MOD. */
struct {
struct ofputil_group_mod *group_mod;
struct ofpbuf bands;
};
};
};
This required further changes elsewhere, but they were straightforward.
Anyway, I folded in the following incremental and applied this to
master. Thanks!
diff --git a/lib/ofp-print.c b/lib/ofp-print.c
index b2d3b4f..d0c94ce 100644
--- a/lib/ofp-print.c
+++ b/lib/ofp-print.c
@@ -1185,21 +1185,9 @@ ofp_print_meter_config(struct ds *s, const struct
ofputil_meter_config *mc)
}
static void
-ofp_print_meter_mod(struct ds *s, const struct ofp_header *oh)
+ofp_print_meter_mod__(struct ds *s, const struct ofputil_meter_mod *mm)
{
- struct ofputil_meter_mod mm;
- struct ofpbuf bands;
- enum ofperr error;
-
- ofpbuf_init(&bands, 64);
- error = ofputil_decode_meter_mod(oh, &mm, &bands);
- if (error) {
- ofpbuf_uninit(&bands);
- ofp_print_error(s, error);
- return;
- }
-
- switch (mm.command) {
+ switch (mm->command) {
case OFPMC13_ADD:
ds_put_cstr(s, " ADD ");
break;
@@ -1210,10 +1198,26 @@ ofp_print_meter_mod(struct ds *s, const struct
ofp_header *oh)
ds_put_cstr(s, " DEL ");
break;
default:
- ds_put_format(s, " cmd:%d ", mm.command);
+ ds_put_format(s, " cmd:%d ", mm->command);
}
- ofp_print_meter_config(s, &mm.meter);
+ ofp_print_meter_config(s, &mm->meter);
+}
+
+static void
+ofp_print_meter_mod(struct ds *s, const struct ofp_header *oh)
+{
+ struct ofputil_meter_mod mm;
+ struct ofpbuf bands;
+ enum ofperr error;
+
+ ofpbuf_init(&bands, 64);
+ error = ofputil_decode_meter_mod(oh, &mm, &bands);
+ if (error) {
+ ofp_print_error(s, error);
+ } else {
+ ofp_print_meter_mod__(s, &mm);
+ }
ofpbuf_uninit(&bands);
}
@@ -2333,7 +2337,8 @@ ofp_print_bucket_id(struct ds *s, const char *label,
uint32_t bucket_id,
static void
ofp_print_group(struct ds *s, uint32_t group_id, uint8_t type,
- struct ovs_list *p_buckets, struct ofputil_group_props *props,
+ const struct ovs_list *p_buckets,
+ const struct ofputil_group_props *props,
enum ofp_version ofp_version, bool suppress_type)
{
struct ofputil_bucket *bucket;
@@ -2529,22 +2534,15 @@ ofp_print_group_features(struct ds *string, const
struct ofp_header *oh)
}
static void
-ofp_print_group_mod(struct ds *s, const struct ofp_header *oh)
+ofp_print_group_mod__(struct ds *s, enum ofp_version ofp_version,
+ const struct ofputil_group_mod *gm)
{
- struct ofputil_group_mod gm;
- int error;
bool bucket_command = false;
- error = ofputil_decode_group_mod(oh, &gm);
- if (error) {
- ofp_print_error(s, error);
- return;
- }
-
ds_put_char(s, '\n');
ds_put_char(s, ' ');
- switch (gm.command) {
+ switch (gm->command) {
case OFPGC11_ADD:
ds_put_cstr(s, "ADD");
break;
@@ -2568,17 +2566,31 @@ ofp_print_group_mod(struct ds *s, const struct
ofp_header *oh)
break;
default:
- ds_put_format(s, "cmd:%"PRIu16"", gm.command);
+ ds_put_format(s, "cmd:%"PRIu16"", gm->command);
}
ds_put_char(s, ' ');
if (bucket_command) {
ofp_print_bucket_id(s, "command_bucket_id:",
- gm.command_bucket_id, oh->version);
+ gm->command_bucket_id, ofp_version);
}
- ofp_print_group(s, gm.group_id, gm.type, &gm.buckets, &gm.props,
- oh->version, bucket_command);
+ ofp_print_group(s, gm->group_id, gm->type, &gm->buckets, &gm->props,
+ ofp_version, bucket_command);
+}
+
+static void
+ofp_print_group_mod(struct ds *s, const struct ofp_header *oh)
+{
+ struct ofputil_group_mod gm;
+ int error;
+
+ error = ofputil_decode_group_mod(oh, &gm);
+ if (error) {
+ ofp_print_error(s, error);
+ return;
+ }
+ ofp_print_group_mod__(s, oh->version, &gm);
ofputil_bucket_list_destroy(&gm.buckets);
}
@@ -3057,7 +3069,6 @@ ofp_print_requestforward(struct ds *string, const struct
ofp_header *oh)
{
struct ofputil_requestforward rf;
enum ofperr error;
- enum ofptype type;
error = ofputil_decode_requestforward(oh, &rf);
if (error) {
@@ -3065,21 +3076,20 @@ ofp_print_requestforward(struct ds *string, const
struct ofp_header *oh)
return;
}
- error = ofptype_decode(&type, rf.request);
- if (error) {
- ofp_print_error(string, error);
- return;
- }
-
ds_put_cstr(string, " reason=");
- if (type == OFPTYPE_GROUP_MOD) {
+ switch (rf.reason) {
+ case OFPRFR_GROUP_MOD:
ds_put_cstr(string, "group_mod");
- ofp_print_group_mod(string, rf.request);
- } else if (type == OFPTYPE_METER_MOD) {
+ ofp_print_group_mod__(string, oh->version, rf.group_mod);
+ break;
+
+ case OFPRFR_METER_MOD:
ds_put_cstr(string, "meter_mod");
- ofp_print_meter_mod(string, rf.request);
+ ofp_print_meter_mod__(string, rf.meter_mod);
+ break;
}
+ ofputil_destroy_requestforward(&rf);
}
static void
diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index 05ee6bb..d90cca8 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -5376,106 +5376,125 @@ ofputil_decode_role_status(const struct ofp_header
*oh,
return 0;
}
-/* Re-encodes the "GROUP_MOD" or "METER_MOD" request header for the
- * 'protocol' in the ofconn. */
-enum ofperr
-ofputil_re_encode_requestforward(uint8_t reason,
- const struct ofp_header *request,
- struct ofp_header **requestp,
- enum ofputil_protocol protocol)
+/* Encodes 'rf' according to 'protocol', and returns the encoded message.
+ * 'protocol' must be for OpenFlow 1.4 or later. */
+struct ofpbuf *
+ofputil_encode_requestforward(const struct ofputil_requestforward *rf,
+ enum ofputil_protocol protocol)
{
- enum ofp_version version;
- struct ofp_header *rf;
- struct ofpbuf *buf;
- enum ofperr error;
-
- version = ofputil_protocol_to_ofp_version(protocol);
- switch (reason) {
- case OFPRFR_GROUP_MOD: {
- struct ofputil_group_mod gm;
+ enum ofp_version ofp_version = ofputil_protocol_to_ofp_version(protocol);
+ struct ofpbuf *inner;
- error = ofputil_decode_group_mod(request, &gm);
- if (error) {
- return error;
- }
- buf = ofputil_encode_group_mod(version, &gm);
- ofputil_bucket_list_destroy(&gm.buckets);
+ switch (rf->reason) {
+ case OFPRFR_GROUP_MOD:
+ inner = ofputil_encode_group_mod(ofp_version, rf->group_mod);
break;
- }
- case OFPRFR_METER_MOD: {
- struct ofputil_meter_mod mm;
- struct ofpbuf bands;
- ofpbuf_init(&bands, 64);
- error = ofputil_decode_meter_mod(request, &mm, &bands);
- if (error) {
- ofpbuf_uninit(&bands);
- return error;
- }
- buf = ofputil_encode_meter_mod(version, &mm);
- ofpbuf_uninit(&bands);
+ case OFPRFR_METER_MOD:
+ inner = ofputil_encode_meter_mod(ofp_version, rf->meter_mod);
break;
- }
+
default:
OVS_NOT_REACHED();
}
- rf = buf->data;
- rf->length = htons(buf->size);
- rf->xid = request->xid;
- *requestp = rf;
- return 0;
-}
-/* Encodes "request forward" message encapsulating "GROUP_MOD" or "METER_MOD"
- * request in 'rf'. The OpenFlow version of OFPT_REQUESTFORWARD message and
- * 'rf->request' should be same.
- * 'rf->request' is not converted from host to network byte order because
- * "GROUP_MOD" or "METER_MOD" request header in 'rf' is directly encapsulated
- * in request forward message. */
-struct ofpbuf *
-ofputil_encode_requestforward(const struct ofputil_requestforward *rf)
-{
- struct ofpbuf *buf;
+ struct ofp_header *inner_oh = inner->data;
+ inner_oh->xid = rf->xid;
+ inner_oh->length = htons(inner->size);
- buf = ofpraw_alloc_xid(OFPRAW_OFPT14_REQUESTFORWARD, rf->request->version,
- htonl(0), ntohs(rf->request->length));
- ofpbuf_put(buf, rf->request, ntohs(rf->request->length));
- return buf;
+ struct ofpbuf *outer = ofpraw_alloc_xid(OFPRAW_OFPT14_REQUESTFORWARD,
+ ofp_version, htonl(0),
+ inner->size);
+ ofpbuf_put(outer, inner->data, inner->size);
+ ofpbuf_delete(inner);
+
+ return outer;
}
-/* Converts OpenFlow request forward message 'oh' into an abstract request
- * forward in 'rf'. Returns 0 if successful, otherwise an OpenFlow error
- * code. */
+/* Decodes OFPT_REQUESTFORWARD message 'outer'. On success, puts the decoded
+ * form into '*rf' and returns 0, and the caller is later responsible for
+ * freeing the content of 'rf', with ofputil_destroy_requestforward(rf). On
+ * failure, returns an ofperr and '*rf' is indeterminate. */
enum ofperr
-ofputil_decode_requestforward(const struct ofp_header *oh,
+ofputil_decode_requestforward(const struct ofp_header *outer,
struct ofputil_requestforward *rf)
{
struct ofpbuf b;
- enum ofpraw raw;
enum ofperr error;
- size_t inner_len;
- ofpbuf_use_const(&b, oh, ntohs(oh->length));
- error = ofpraw_pull(&raw, &b);
- if (error) {
- return error;
- }
-
- ovs_assert(raw == OFPRAW_OFPT14_REQUESTFORWARD);
+ ofpbuf_use_const(&b, outer, ntohs(outer->length));
- rf->request = b.data;
+ /* Skip past outer message. */
+ enum ofpraw outer_raw = ofpraw_pull_assert(&b);
+ ovs_assert(outer_raw == OFPRAW_OFPT14_REQUESTFORWARD);
- if (rf->request->version != oh->version) {
- return OFPERR_OFPBRC_BAD_VERSION;
+ /* Validate inner message. */
+ if (b.size < sizeof(struct ofp_header)) {
+ return OFPERR_OFPBFC_MSG_BAD_LEN;
}
- inner_len = ntohs(rf->request->length);
+ const struct ofp_header *inner = b.data;
+ unsigned int inner_len = ntohs(inner->length);
if (inner_len < sizeof(struct ofp_header) || inner_len > b.size) {
return OFPERR_OFPBFC_MSG_BAD_LEN;
}
+ if (inner->version != outer->version) {
+ return OFPERR_OFPBRC_BAD_VERSION;
+ }
+
+ /* Parse inner message. */
+ enum ofptype type;
+ error = ofptype_decode(&type, inner);
+ if (error) {
+ return error;
+ }
+
+ rf->xid = inner->xid;
+ if (type == OFPTYPE_GROUP_MOD) {
+ rf->reason = OFPRFR_GROUP_MOD;
+ rf->group_mod = xmalloc(sizeof *rf->group_mod);
+ error = ofputil_decode_group_mod(inner, rf->group_mod);
+ if (error) {
+ free(rf->group_mod);
+ return error;
+ }
+ } else if (type == OFPTYPE_METER_MOD) {
+ rf->reason = OFPRFR_METER_MOD;
+ rf->meter_mod = xmalloc(sizeof *rf->meter_mod);
+ ofpbuf_init(&rf->bands, 64);
+ error = ofputil_decode_meter_mod(inner, rf->meter_mod, &rf->bands);
+ if (error) {
+ free(rf->meter_mod);
+ ofpbuf_uninit(&rf->bands);
+ return error;
+ }
+ } else {
+ return OFPERR_OFPBFC_MSG_UNSUP;
+ }
return 0;
}
+/* Frees the content of 'rf', which should have been initialized through a
+ * successful call to ofputil_decode_requestforward(). */
+void
+ofputil_destroy_requestforward(struct ofputil_requestforward *rf)
+{
+ if (!rf) {
+ return;
+ }
+
+ switch (rf->reason) {
+ case OFPRFR_GROUP_MOD:
+ ofputil_uninit_group_mod(rf->group_mod);
+ free(rf->group_mod);
+ break;
+
+ case OFPRFR_METER_MOD:
+ ofpbuf_uninit(&rf->bands);
+ free(rf->meter_mod);
+ }
+}
+
/* Table stats. */
/* OpenFlow 1.0 and 1.1 don't distinguish between a field that cannot be
diff --git a/lib/ofp-util.h b/lib/ofp-util.h
index 7589f51..cbd6175 100644
--- a/lib/ofp-util.h
+++ b/lib/ofp-util.h
@@ -861,22 +861,6 @@ struct ofpbuf *ofputil_encode_role_status(
enum ofperr ofputil_decode_role_status(const struct ofp_header *oh,
struct ofputil_role_status *rs);
-struct ofputil_requestforward {
- const struct ofp_header *request;
-};
-
-enum ofperr
-ofputil_re_encode_requestforward(uint8_t reason,
- const struct ofp_header *request,
- struct ofp_header **requestp,
- enum ofputil_protocol protocol);
-struct ofpbuf *
-ofputil_encode_requestforward(const struct ofputil_requestforward *rf);
-
-enum ofperr
-ofputil_decode_requestforward(const struct ofp_header *oh,
- struct ofputil_requestforward *rf);
-
/* Abstract table stats.
*
* This corresponds to the OpenFlow 1.3 table statistics structure, which only
@@ -1255,4 +1239,25 @@ struct ofpbuf *ofputil_encode_get_async_config(const
struct ofp_header *,
uint32_t master[OAM_N_TYPES],
uint32_t slave[OAM_N_TYPES]);
+struct ofputil_requestforward {
+ ovs_be32 xid;
+ enum ofp14_requestforward_reason reason;
+ union {
+ /* reason == OFPRFR_METER_MOD. */
+ struct ofputil_meter_mod *meter_mod;
+
+ /* reason == OFPRFR_GROUP_MOD. */
+ struct {
+ struct ofputil_group_mod *group_mod;
+ struct ofpbuf bands;
+ };
+ };
+};
+
+struct ofpbuf *ofputil_encode_requestforward(
+ const struct ofputil_requestforward *, enum ofputil_protocol);
+enum ofperr ofputil_decode_requestforward(const struct ofp_header *,
+ struct ofputil_requestforward *);
+void ofputil_destroy_requestforward(struct ofputil_requestforward *);
+
#endif /* ofp-util.h */
diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c
index 60f3d9a..ef2c06f 100644
--- a/ofproto/connmgr.c
+++ b/ofproto/connmgr.c
@@ -1714,46 +1714,21 @@ connmgr_send_port_status(struct connmgr *mgr, struct
ofconn *source,
* controller OFPT_GROUP_MOD and OFPT_METER_MOD, specify 'source' as the
* controller connection that sent the request; otherwise, specify 'source'
* as NULL. */
-enum ofperr
-connmgr_send_requestforward(struct connmgr *mgr, struct ofconn *source,
- uint8_t reason, const struct ofp_header *oh)
+void
+connmgr_send_requestforward(struct connmgr *mgr, const struct ofconn *source,
+ const struct ofputil_requestforward *rf)
{
- struct ofputil_requestforward rf;
struct ofconn *ofconn;
- struct ofp_header *request = NULL;
- enum ofperr error;
LIST_FOR_EACH (ofconn, node, &mgr->all_conns) {
- if (ofconn_receives_async_msg(ofconn, OAM_REQUESTFORWARD, reason)) {
- struct ofpbuf *msg;
-
- rf.request = oh;
- /* For OpenFlow 1.4 and later, it generates OFPT_REQUESTFORWARD
- * for OFPT_GROUP_MOD and OFPT_METER_MOD, but not back to the
- * originating controller. In a single-controller environment, in
- * particular, this means that it will never generate
- * OFPT_REQUESTFORWARD for OFPT_GROUP_MOD and OFPT_METER_MOD at
- * all. */
- if (rconn_get_version(ofconn->rconn) < OFP14_VERSION
- || ofconn == source) {
- continue;
- }
- /* When the version of GROUP_MOD or METER_MOD request is not same
- * as that of ofconn, then re-encode the GROUP_MOD or METER_MOD
- * request for the protocol in use. */
- if (oh->version != rconn_get_version(ofconn->rconn)) {
- error = ofputil_re_encode_requestforward(reason, oh, &request,
- ofconn_get_protocol(ofconn));
- if (error) {
- return error;
- }
- rf.request = request;
- }
- msg = ofputil_encode_requestforward(&rf);
- ofconn_send(ofconn, msg, NULL);
+ if (ofconn_receives_async_msg(ofconn, OAM_REQUESTFORWARD, rf->reason)
+ && rconn_get_version(ofconn->rconn) >= OFP14_VERSION
+ && ofconn != source) {
+ enum ofputil_protocol protocol = ofconn_get_protocol(ofconn);
+ ofconn_send(ofconn, ofputil_encode_requestforward(rf, protocol),
+ NULL);
}
}
- return 0;
}
/* Sends an OFPT_FLOW_REMOVED or NXT_FLOW_REMOVED message based on 'fr' to
diff --git a/ofproto/connmgr.h b/ofproto/connmgr.h
index 2de7f6f..8048424 100644
--- a/ofproto/connmgr.h
+++ b/ofproto/connmgr.h
@@ -168,9 +168,8 @@ void connmgr_send_packet_in(struct connmgr *,
void ofconn_send_role_status(struct ofconn *ofconn, uint32_t role,
uint8_t reason);
-enum ofperr
-connmgr_send_requestforward(struct connmgr *, struct ofconn *source,
- uint8_t reason, const struct ofp_header *);
+void connmgr_send_requestforward(struct connmgr *, const struct ofconn *source,
+ const struct ofputil_requestforward *);
/* Fail-open settings. */
enum ofproto_fail_mode connmgr_get_fail_mode(const struct connmgr *);
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 99b743c..c1301b5 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -5909,8 +5909,11 @@ handle_meter_mod(struct ofconn *ofconn, const struct
ofp_header *oh)
}
if (!error) {
- error = connmgr_send_requestforward(ofproto->connmgr, ofconn,
- OFPRFR_METER_MOD, oh);
+ struct ofputil_requestforward rf;
+ rf.xid = oh->xid;
+ rf.reason = OFPRFR_METER_MOD;
+ rf.meter_mod = &mm;
+ connmgr_send_requestforward(ofproto->connmgr, ofconn, &rf);
}
exit_free_bands:
@@ -6609,8 +6612,11 @@ handle_group_mod(struct ofconn *ofconn, const struct
ofp_header *oh)
}
if (!error) {
- error = connmgr_send_requestforward(ofproto->connmgr, ofconn,
- OFPRFR_GROUP_MOD, oh);
+ struct ofputil_requestforward rf;
+ rf.xid = oh->xid;
+ rf.reason = OFPRFR_GROUP_MOD;
+ rf.group_mod = &gm;
+ connmgr_send_requestforward(ofproto->connmgr, ofconn, &rf);
}
return error;
}
diff --git a/tests/ofproto.at b/tests/ofproto.at
index 38d8ba6..7e80293 100644
--- a/tests/ofproto.at
+++ b/tests/ofproto.at
@@ -2904,7 +2904,7 @@ dnl command is sent from one controller and the reply is
received by
dnl other controllers.
AT_SETUP([ofproto - requestforward (OpenFlow 1.4)])
OVS_VSWITCHD_START
-ON_EXIT([kill `cat c1.pid c2.pid c3.pid`])
+on_exit 'kill `cat c1.pid c2.pid c3.pid`'
# Start two ovs-ofctl controller processes.
AT_CAPTURE_FILE([monitor1.log])
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
