On Tue, Oct 08, 2013 at 02:19:39PM -0700, Ben Pfaff wrote: > On Tue, Oct 08, 2013 at 12:26:22PM +0300, Alexandru Copot wrote: > > Signed-off-by: Alexandru Copot <[email protected]> > > Cc: Daniel Baluta <[email protected]> > > Thanks for the patch. > > "git am" says: > > Applying: lib/ofp-util: add decoder for role status asynchronous message > /home/blp/ovs/.git/rebase-apply/patch:27: trailing whitespace. > const struct ofp14_role_status *r = b.l3; > warning: 1 line adds whitespace errors. > > GCC says: > > ../lib/ofp-util.c: In function 'ofputil_decode_role_status': > ../lib/ofp-util.c:3939:5: error: ISO C90 forbids mixed declarations and > code [-Werror=declaration-after-statement] > > > +enum ofperr > > +ofputil_decode_role_status(const struct ofp_header *oh, > > + struct ofputil_role_status *rs) > > +{ > > + struct ofpbuf b; > > + enum ofpraw raw; > > + > > + ofpbuf_use_const(&b, oh, ntohs(oh->length)); > > + raw = ofpraw_pull_assert(&b); > > We always put {} around 'if' blocks. But I would just use > ovs_assert() here instead of an 'if': > > + if (raw != OFPRAW_OFPT14_ROLE_STATUS) > > + NOT_REACHED(); > > + > > + const struct ofp14_role_status *r = b.l3; > > + if (r->role != htonl(OFPCR12_ROLE_NOCHANGE) && > > + r->role != htonl(OFPCR12_ROLE_EQUAL) && > > + r->role != htonl(OFPCR12_ROLE_MASTER) && > > + r->role != htonl(OFPCR12_ROLE_SLAVE)) { > > This should only be indented 4 spaces beyond the 'if', as usual: > > + return OFPERR_OFPRRFC_BAD_ROLE; > > + } > > + > > + rs->role = ntohl(r->role); > > + rs->generation_id = ntohll(r->generation_id); > > + > > + return 0; > > +}
Also it appears that ofputil_decode_role_status() does not set rs->reason or check that the reason is valid. (The latter might be OK except that the ofp-print code in the final patch assert-fails if the reason is not valid.) _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
