On May 7, 2012, at 4:59 PM, Ben Pfaff wrote: > On Mon, May 07, 2012 at 11:56:50AM -0700, Justin Pettit wrote: >> Add function to determine whether the max number of ports are contains >> in a Features Reply. If so, it removes the port list, since it may be >> incomplete. This function will be used in a later commit. >> >> Signed-off-by: Justin Pettit <[email protected]> > > Now we have several bits of code that translate an ofp version into a > struct size. I count three, in ofputil_decode_switch_features(), > max_ports_in_features(), and ofputil_count_phy_ports(). It'd be > better to just have a single helper to do that.
Yeah, the thought had crossed my mind. :-) I've attached a commit to do that, which would follow this one in the series. (Congratulations on finally getting referenced in the commit log!) > In ofputil_switch_features_ports_trunc() you can just call > update_openflow_length() instead of inlining it as: >> + osf->header.length = htons(sizeof(*osf)); Nice. Thanks. --Justin ----------------- commit cb686d6b757eb4000a27b920cc65721753462877 Author: Justin Pettit <[email protected]> Date: Tue May 8 00:01:11 2012 -0700 ofp-util: Factor out determining physical port size. There are a few places where we determine the size of a physical port structure based on the OpenFlow version. Use a helper function to do that. Suggested-by: Ben Pfaff <[email protected]> Signed-off-by: Justin Pettit <[email protected]> diff --git a/lib/ofp-util.c b/lib/ofp-util.c index 69e2b17..48be774 100644 --- a/lib/ofp-util.c +++ b/lib/ofp-util.c @@ -2371,6 +2371,13 @@ ofputil_decode_ofp11_port(struct ofputil_phy_port *pp, return 0; } +static size_t +ofputil_get_phy_port_size(uint8_t ofp_version) +{ + return ofp_version == OFP10_VERSION ? sizeof(struct ofp10_phy_port) + : sizeof(struct ofp11_port); +} + static void ofputil_encode_ofp10_phy_port(const struct ofputil_phy_port *pp, struct ofp10_phy_port *opp) @@ -2543,20 +2550,17 @@ ofputil_decode_switch_features(const struct ofp_switch_f features->n_tables = osf->n_tables; features->capabilities = ntohl(osf->capabilities) & OFPC_COMMON; - if (osf->header.version == OFP10_VERSION) { - if (b->size % sizeof(struct ofp10_phy_port)) { - return OFPERR_OFPBRC_BAD_LEN; - } + if (b->size % ofputil_get_phy_port_size(osf->header.version)) { + return OFPERR_OFPBRC_BAD_LEN; + } + + if (osf->header.version == OFP10_VERSION) { if (osf->capabilities & htonl(OFPC10_STP)) { features->capabilities |= OFPUTIL_C_STP; } features->actions = decode_action_bits(osf->actions, of10_action_bits); } else if (osf->header.version == OFP11_VERSION) { - if (b->size % sizeof(struct ofp11_port)) { - return OFPERR_OFPBRC_BAD_LEN; - } - if (osf->capabilities & htonl(OFPC11_GROUP_STATS)) { features->capabilities |= OFPUTIL_C_GROUP_STATS; } @@ -2572,10 +2576,7 @@ ofputil_decode_switch_features(const struct ofp_switch_fe static bool max_ports_in_features(const struct ofp_switch_features *osf) { - size_t pp_size = osf->header.version == OFP10_VERSION ? - sizeof(struct ofp10_phy_port) : - sizeof(struct ofp11_port); - + size_t pp_size = ofputil_get_phy_port_size(osf->header.version); return ntohs(osf->header.length) + pp_size > UINT16_MAX; } @@ -3403,9 +3404,7 @@ ofputil_pull_phy_port(uint8_t ofp_version, struct ofpbuf * * 'ofp_version', returns the number of elements. */ size_t ofputil_count_phy_ports(uint8_t ofp_version, struct ofpbuf *b) { - return (ofp_version == OFP10_VERSION - ? b->size / sizeof(struct ofp10_phy_port) - : b->size / sizeof(struct ofp11_port)); + return b->size / ofputil_get_phy_port_size(ofp_version); } static enum ofperr _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
