Hi Ben, Thanks for the review. Yes, I find this approach better where data is decoded only once.
I have a doubt regarding the following structure: 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; }; }; }; Here, 'bands' are included as a part of group_mod, but as per the code and specification, bands are part of meter_mod. Please help me understand why 'bands' have been included with group_mod. As per my understanding, the structure should be in this manner: struct ofputil_requestforward { ovs_be32 xid; enum ofp14_requestforward_reason reason; union { /* reason == OFPRFR_METER_MOD. */ struct { struct ofputil_meter_mod *meter_mod; struct ofpbuf bands; }; /* reason == OFPRFR_GROUP_MOD. */ struct ofputil_group_mod *group_mod; }; }; Kindly validate my understanding. Thanks & Regards Niti Rohilla Tata Consultancy Services On Thu, Sep 10, 2015 at 1:48 AM, Ben Pfaff <b...@nicira.com> wrote: > 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