On Wed, Nov 06, 2013 at 10:45:31PM +0800, Alexander Wu wrote: > > V2: > Restructure implement of OFPMP_TABLE_FEATURES > Change decode_*_raw to normalized pull functions > > 1. add macros and funcs to en/decode table features > 2. restructure OFPMP_TABLE_FEATURES en/decode function > now they act like others. > 3. Change big array to defines.(oxm, table_feature_prop) > Fix some names, now they're more meaningful. > 4. use macros to restructure implement > 5. Restructure get_* to more effective ones. (table_features, oxm) > 6. remove useless array and prototype > > Simon Horman suggestions: > Fix function paras alignment. > Fix hard coding to marco. > Fix VLOG calls with rl. > Fix CodingStyle: > max chars per line - 79 > Delete useless blank line. > Restructure implement by macro. > Seperate the patch of SET and GET. - I'll do it later. > > V1: > ofp-util: Implement the encode/decode Table Features functions > > Implement the encode/decode table features msgs function, and > NOTE that we implement the decode functions *_raw, maybe we > should change it the ofpbuf_pull? > > Signed-off-by: Alexander Wu <alexander...@huawei.com> > --- > lib/ofp-util.c | 723 > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > lib/ofp-util.h | 153 ++++++++++++ > 2 files changed, 876 insertions(+), 0 deletions(-) > > diff --git a/lib/ofp-util.c b/lib/ofp-util.c > index 9645e04..365616e 100644 > --- a/lib/ofp-util.c > +++ b/lib/ofp-util.c > @@ -4066,6 +4066,729 @@ ofputil_encode_port_mod(const struct ofputil_port_mod > *pm, > return b; > } > > +static int > +tfprop_count_n_elem(uint32_t *n_elem, uint16_t length, uint16_t elem_size)
I think that enum ofperr would be a better return type. > +{ > + int n = 0; > + if (length % elem_size) > + return OFPERR_OFPTFFC_BAD_LEN; > + > + n = length / elem_size; > + *n_elem = n; > + return 0; > +} > + > +#define OVS_OXM \ > + DEFINE_OXM(OXM_OF_IN_PORT, "IN_PORT") \ > + DEFINE_OXM(OXM_OF_IN_PHY_PORT, "IN_PHY_PORT") \ > + DEFINE_OXM(OXM_OF_METADATA, "METADATA") \ > + DEFINE_OXM(OXM_OF_ETH_DST, "ETH_DST") \ > + DEFINE_OXM(OXM_OF_ETH_SRC, "ETH_SRC") \ > + DEFINE_OXM(OXM_OF_ETH_TYPE, "ETH_TYPE") \ > + DEFINE_OXM(OXM_OF_VLAN_VID, "VLAN_VID") \ > + DEFINE_OXM(OXM_OF_VLAN_PCP, "VLAN_PCP") \ > + DEFINE_OXM(OXM_OF_IP_DSCP, "IP_DSCP") \ > + DEFINE_OXM(OXM_OF_IP_ECN, "IP_ECN") \ > + DEFINE_OXM(OXM_OF_IP_PROTO, "IP_PROTO") \ > + DEFINE_OXM(OXM_OF_IPV4_SRC, "IPV4_SRC") \ > + DEFINE_OXM(OXM_OF_IPV4_DST, "IPV4_DST") \ > + DEFINE_OXM(OXM_OF_TCP_SRC, "TCP_SRC") \ > + DEFINE_OXM(OXM_OF_TCP_DST, "TCP_DST") \ > + DEFINE_OXM(OXM_OF_UDP_SRC, "UDP_SRC") \ > + DEFINE_OXM(OXM_OF_UDP_DST, "UDP_DST") \ > + DEFINE_OXM(OXM_OF_SCTP_SRC, "SCTP_SRC") \ > + DEFINE_OXM(OXM_OF_SCTP_DST, "SCTP_DST") \ > + DEFINE_OXM(OXM_OF_ICMPV4_TYPE, "ICMPV4_TYPE") \ > + DEFINE_OXM(OXM_OF_ICMPV4_CODE, "ICMPV4_CODE") \ > + DEFINE_OXM(OXM_OF_ARP_OP, "ARP_OP") \ > + DEFINE_OXM(OXM_OF_ARP_SPA, "ARP_SPA") \ > + DEFINE_OXM(OXM_OF_ARP_TPA, "ARP_TPA") \ > + DEFINE_OXM(OXM_OF_ARP_SHA, "ARP_SHA") \ > + DEFINE_OXM(OXM_OF_ARP_THA, "ARP_THA") \ > + DEFINE_OXM(OXM_OF_IPV6_SRC, "IPV6_SRC") \ > + DEFINE_OXM(OXM_OF_IPV6_DST, "IPV6_DST") \ > + DEFINE_OXM(OXM_OF_IPV6_FLABEL, "IPV6_FLABEL") \ > + DEFINE_OXM(OXM_OF_ICMPV6_TYPE, "ICMPV6_TYPE") \ > + DEFINE_OXM(OXM_OF_ICMPV6_CODE, "ICMPV6_CODE") \ > + DEFINE_OXM(OXM_OF_IPV6_ND_TARGET, "IPV6_ND_TARGET") \ > + DEFINE_OXM(OXM_OF_IPV6_ND_SLL, "IPV6_ND_SLL") \ > + DEFINE_OXM(OXM_OF_IPV6_ND_TLL, "IPV6_ND_TLL") \ > + DEFINE_OXM(OXM_OF_MPLS_LABEL, "MPLS_LABEL") \ > + DEFINE_OXM(OXM_OF_MPLS_TC, "MPLS_TC") \ > + DEFINE_OXM(OXM_OF_MPLS_BOS, "MPLS_BOS") \ > + DEFINE_OXM(OXM_OF_TUNNEL_ID, "TUNNEL_ID") \ > + DEFINE_OXM(OXM_OF_IPV6_EXTHDR, "IPV6_EXTHDR") > + > +enum { > +#define DEFINE_OXM(ENUM, NAME) + 1 > + N_OVS_OXM = OVS_OXM > +#undef DEFINE_OXM > +}; > + > +struct oxm_info oxm_info[] = { > +#define DEFINE_OXM(ENUM, NAME) {ENUM, NAME}, > +OVS_OXM > +#undef DEFINE_OXM > +}; > + > +int get_oxm_num(void) > +{ > + return N_OVS_OXM; > +} > + > +char *get_oxm_name(uint32_t type) > +{ > + switch (type) { > +#define DEFINE_OXM(ENUM, NAME) \ > + case ENUM: \ > + return NAME; > +OVS_OXM > +#undef DEFINE_OXM > + default: > + return NULL; > + } > +} > + > +struct table_feature_prop { > + uint16_t type; I think that enum ofp13_table_feature_prop_type would be a better type for the 'type' field. > + uint16_t length; > + char *name; > +}; > + > +#define OVS_TABLE_FEATURE_PROPS \ > + DEFINE_TFPROP(OFPTFPT13_INSTRUCTIONS, \ > + struct ofp11_instruction, instruction_array, \ > + "OFPTFPT13_INSTRUCTIONS") \ > + DEFINE_TFPROP(OFPTFPT13_INSTRUCTIONS_MISS, \ > + struct ofp11_instruction, instruction_array, \ > + "OFPTFPT13_INSTRUCTIONS_MISS") \ > + DEFINE_TFPROP(OFPTFPT13_NEXT_TABLES, \ > + uint8_t, null, \ > + "OFPTFPT13_NEXT_TABLES") \ > + DEFINE_TFPROP(OFPTFPT13_NEXT_TABLES_MISS, \ > + uint8_t, null, \ > + "OFPTFPT13_NEXT_TABLES_MISS") \ > + DEFINE_TFPROP(OFPTFPT13_WRITE_ACTIONS, \ > + struct ofp_action_header, action_array, \ > + "OFPTFPT13_WRITE_ACTIONS") \ > + DEFINE_TFPROP(OFPTFPT13_WRITE_ACTIONS_MISS, \ > + struct ofp_action_header, action_array, \ > + "OFPTFPT13_WRITE_ACTIONS_MISS") \ > + DEFINE_TFPROP(OFPTFPT13_APPLY_ACTIONS, \ > + struct ofp_action_header, action_array, \ > + "OFPTFPT13_APPLY_ACTIONS") \ > + DEFINE_TFPROP(OFPTFPT13_APPLY_ACTIONS_MISS, \ > + struct ofp_action_header, action_array, \ > + "OFPTFPT13_APPLY_ACTIONS_MISS") \ > + DEFINE_TFPROP(OFPTFPT13_MATCH, \ > + ovs_be32, be32_array, \ > + "OFPTFPT13_MATCH") \ > + DEFINE_TFPROP(OFPTFPT13_WILDCARDS, \ > + ovs_be32, be32_array, \ > + "OFPTFPT13_WILDCARDS") \ > + DEFINE_TFPROP(OFPTFPT13_WRITE_SETFIELD, \ > + ovs_be32, be32_array, \ > + "OFPTFPT13_WRITE_SETFIELD") \ > + DEFINE_TFPROP(OFPTFPT13_WRITE_SETFIELD_MISS, \ > + ovs_be32, be32_array, \ > + "OFPTFPT13_WRITE_SETFIELD_MISS") \ > + DEFINE_TFPROP(OFPTFPT13_APPLY_SETFIELD, \ > + ovs_be32, be32_array, \ > + "OFPTFPT13_APPLY_SETFIELD") \ > + DEFINE_TFPROP(OFPTFPT13_APPLY_SETFIELD_MISS, \ > + ovs_be32, be32_array, \ > + "OFPTFPT13_APPLY_SETFIELD_MISS") > +/* I think it would be best to remove the OFPTFPT13_EXPERIMENTER and OFPTFPT13_EXPERIMENTER_MISS entries. > + DEFINE_TFPROP(OFPTFPT13_EXPERIMENTER, \ > + ovs_be32, be32_array, \ > + "OFPTFPT13_EXPERIMENTER") \ > + DEFINE_TFPROP(OFPTFPT13_EXPERIMENTER_MISS, \ > + ovs_be32, be32_array, \ > + "OFPTFPT13_EXPERIMENTER_MISS") > +*/ > + > +char *table_feature_prop_get_name(uint16_t type) I think that enum ofp13_table_feature_prop_type would be a better type for the 'type' parameter. > +{ > + switch (type) { > +#define DEFINE_TFPROP(ENUM, STRUCT, TAG, NAME) \ > + case ENUM: \ > + return NAME; > +OVS_TABLE_FEATURE_PROPS > +#undef DEFINE_TFPROP > + default: > + return NULL; > + } > +} > + > +uint16_t table_feature_prop_get_length(uint16_t type) I think that enum ofp13_table_feature_prop_type would be a better type for the 'type' parameter. And I think that size_t would be a better return type. > +{ > + switch (type) { > +#define DEFINE_TFPROP(ENUM, STRUCT, TAG, NAME) \ > + case ENUM: \ > + return sizeof(STRUCT); > +OVS_TABLE_FEATURE_PROPS > +#undef DEFINE_TFPROP > + default: > + return 0; > + } > +} > + > +#define TFPROP_ALIGN 8 > +#define MULTIPART_HDR_LEN 8 /* TYPE:2 FLAG:2 PADDING:4 */ > +#define MULTIPART_ALIGN 8 > + > +static int > +tfprop_is_valid(const struct ofp13_table_feature_prop_header *prop, > + size_t n_prop) > +{ > + uint16_t len = ntohs(prop->length); > + return (!(len % 1) Why 1? > + && len >= sizeof *prop > + && (ROUND_UP(len, TFPROP_ALIGN) / TFPROP_ALIGN) <= n_prop); > +} > + > +static inline struct ofp13_table_feature_prop_header * > +tfprop_next(const struct ofp13_table_feature_prop_header *prop) > +{ > + return ((struct ofp13_table_feature_prop_header *) (void *) Is the (void *) necessary? > + ((uint8_t *) prop + ROUND_UP(ntohs(prop->length), > TFPROP_ALIGN))); > +} > + > +/* This macro is careful to check for props with bad lengths. */ > +#define TFPROP_FOR_EACH(ITER, LEFT, PROPS, N_PROPS) \ > + for ((ITER) = (PROPS), (LEFT) = (N_PROPS); \ > + (LEFT) > 0 && tfprop_is_valid(ITER, LEFT); \ > + ((LEFT) -= ROUND_UP(ntohs((ITER)->length), TFPROP_ALIGN) \ > + / TFPROP_ALIGN, \ > + (ITER) = tfprop_next(ITER))) > + > +enum ovs_tfprop_type { > +#define DEFINE_TFPROP(ENUM, STRUCT, TAG, NAME) OVSTFPROP_##ENUM, > + OVS_TABLE_FEATURE_PROPS > +#undef DEFINE_TFPROP > +}; > + > +enum { > +#define DEFINE_TFPROP(ENUM, STRUCT, TAG, NAME) + 1 > + N_OVS_TFPROPS = OVS_TABLE_FEATURE_PROPS > +#undef DEFINE_TFPROP > +}; Do you need a 0 somewhere in the enum above? > + > +struct tfprop_type_info { > + enum ovs_tfprop_type type; > + const char *name; > +}; > + > +static const struct tfprop_type_info prop_info[] = { > +#define DEFINE_TFPROP(ENUM, STRUCT, TAG, NAME) {OVSTFPROP_##ENUM, NAME}, > +OVS_TABLE_FEATURE_PROPS > +#undef DEFINE_TFPROP > +}; > + > +#define OPROP_DATA(OPROP) ((void *)(OPROP + 1)) > +static enum ofperr > +trans_openflow13_tfprop(const struct ofp13_table_feature_prop_header *oprop, > + struct ofputil_table_feature_prop_header *prop) > +{ > + uint32_t data_len; > + uint32_t padding_len; > + > + enum ofperr error = 0; > + uint32_t element_size = 0; > + uint32_t n_elem = 0; > + int i; > + > + prop->type = ntohs(oprop->type); > + prop->length = ntohs(oprop->length); > + data_len = prop->length - sizeof(*oprop); > + > + /* NOTE Special xmalloc */ I'm unsure how this is special. > + prop->data = xmalloc(data_len); > + memcpy(prop->data, (void *)(oprop + 1), data_len); You can use OPROP_DATA() in the line above. > + padding_len = ROUND_UP(prop->length, TFPROP_ALIGN) - prop->length; > + > + element_size = table_feature_prop_get_length(prop->type); > + if (0 == element_size) { if (element_size == 0) { > + return OFPERR_OFPTFFC_BAD_TYPE; > + } else if (data_len % element_size) { > + return OFPERR_OFPTFFC_BAD_LEN; > + } > + > + error = tfprop_count_n_elem(&n_elem, data_len, element_size); > + if (error) { > + return error; > + } It might be cleaner to calculate n_elem in the switch below. But I do not feel strongly about this. > + > + switch (prop->type) { > + case OFPUTIL_INSTRUCTIONS: > + case OFPUTIL_INSTRUCTIONS_MISS: { > + struct ofp11_instruction *oinst = OPROP_DATA(oprop); > + struct ofputil_instruction *inst = prop->data; > + for (i = 0; i < n_elem; i++) { > + inst[i].len = ntohs(oinst[i].len); > + inst[i].type = ntohs(oinst[i].type); > + } > + break; > + } > + case OFPUTIL_NEXT_TABLES: > + case OFPUTIL_NEXT_TABLES_MISS: { > + uint8_t *ontable = OPROP_DATA(oprop); > + uint8_t *ntable = prop->data; > + for (i = 0; i < n_elem; i++) { > + ntable[i] = ontable[i]; > + } > + break; > + } > + case OFPUTIL_WRITE_ACTIONS: > + case OFPUTIL_WRITE_ACTIONS_MISS: > + case OFPUTIL_APPLY_ACTIONS: > + case OFPUTIL_APPLY_ACTIONS_MISS: { > + const struct ofp_action_header *oact = OPROP_DATA(oprop); > + struct ofp_action_header *act = prop->data; > + for (i = 0; i < n_elem; i++) { > + act[i].len = ntohs(oact[i].len); > + act[i].type = ntohs(oact[i].type); > + } > + break; > + } > + case OFPUTIL_MATCH: > + case OFPUTIL_WILDCARDS: > + case OFPUTIL_WRITE_SETFIELD: > + case OFPUTIL_WRITE_SETFIELD_MISS: > + case OFPUTIL_APPLY_SETFIELD: > + case OFPUTIL_APPLY_SETFIELD_MISS: { > + const ovs_be32 *be32 = OPROP_DATA(oprop); > + uint32_t *u32 = prop->data; > + for (i = 0; i < n_elem; i++) { > + u32[i] = ntohl(be32[i]); > + } > + break; > + } > + case OFPUTIL_EXPERIMENTER: > + case OFPUTIL_EXPERIMENTER_MISS: > + default: > + return OFPERR_OFPTFFC_BAD_TYPE; > + } > + return 0; > +} > + > +static enum ofperr > +encode_openflow13_tfprop(struct ofpbuf *reply, > + const struct ofputil_table_feature_prop_header > *prop) > +{ > + uint32_t data_len; > + uint32_t padding_len; > + void *data; > + > + uint32_t n_elem; > + uint16_t element_size; > + enum ofperr error; > + int i; > + struct ofp13_table_feature_prop_header *oprop; > + > + oprop = ofpbuf_put_zeros(reply, sizeof *oprop); > + data_len = prop->length - sizeof *oprop; > + padding_len = ROUND_UP(prop->length, TFPROP_ALIGN) - prop->length; > + > + element_size = table_feature_prop_get_length(prop->type); > + if (0 == element_size) { > + return OFPERR_OFPTFFC_BAD_TYPE; > + } else if (data_len % element_size) { > + return OFPERR_OFPTFFC_BAD_LEN; > + } > + > + error = tfprop_count_n_elem(&n_elem, data_len, element_size); > + if (error) { > + return error; > + } > + > + oprop->type = htons(prop->type); > + oprop->length = htons(prop->length); > + > + data = ofpbuf_put_uninit(reply, data_len + padding_len); > + memset(data, 0, data_len + padding_len); > + > + switch (prop->type) { > + case OFPUTIL_INSTRUCTIONS: > + case OFPUTIL_INSTRUCTIONS_MISS: { > + struct ofp11_instruction *oinst = data; > + struct ofputil_instruction *inst = prop->data; > + for (i = 0; i < n_elem; i++) { > + oinst[i].len = htons(inst[i].len); > + oinst[i].type = htons(inst[i].type); > + } > + break; > + } > + case OFPUTIL_NEXT_TABLES: > + case OFPUTIL_NEXT_TABLES_MISS: { > + uint8_t *ontable = data; > + uint8_t *ntable = prop->data; > + for (i = 0; i < n_elem; i++) { > + ontable[i] = ntable[i]; > + } > + break; > + } > + case OFPUTIL_WRITE_ACTIONS: > + case OFPUTIL_WRITE_ACTIONS_MISS: > + case OFPUTIL_APPLY_ACTIONS: > + case OFPUTIL_APPLY_ACTIONS_MISS: { > + struct ofp_action_header *oact = data; > + struct ofp_action_header *act = prop->data; > + for (i = 0; i < n_elem; i++) { > + oact[i].len = htons(act[i].len); > + oact[i].type = htons(act[i].type); > + } > + break; > + } > + case OFPUTIL_MATCH: > + case OFPUTIL_WILDCARDS: > + case OFPUTIL_WRITE_SETFIELD: > + case OFPUTIL_WRITE_SETFIELD_MISS: > + case OFPUTIL_APPLY_SETFIELD: > + case OFPUTIL_APPLY_SETFIELD_MISS: { > + ovs_be32 *be32 = data; > + uint32_t *u32 = prop->data; > + for (i = 0; i < n_elem; i++) { > + be32[i] = htonl(u32[i]); > + } > + break; > + } > + case OFPUTIL_EXPERIMENTER: > + case OFPUTIL_EXPERIMENTER_MISS: > + default: > + return OFPERR_OFPTFFC_BAD_TYPE; > + } > + return 0; > +} > + > +static enum ofperr > +decode_openflow13_tfprop(const struct ofp13_table_feature_prop_header *prop, > + enum ofp13_table_feature_prop_type *type) > +{ > + uint16_t len = ntohs(prop->length); > + > + switch (prop->type) { > + case CONSTANT_HTONS(OFPTFPT13_EXPERIMENTER): > + return OFPERR_OFPTFFC_BAD_ARGUMENT; I think you can remove the line above. > + case CONSTANT_HTONS(OFPTFPT13_EXPERIMENTER_MISS): > + return OFPERR_OFPTFFC_BAD_ARGUMENT; > + > +#define DEFINE_TFPROP(ENUM, STRUCT, TAG, NAME) \ > + case CONSTANT_HTONS(ENUM): \ > + if (len - sizeof *prop / sizeof(STRUCT)) { \ > + *type = OVSTFPROP_##ENUM; \ > + return 0; \ > + } else { \ > + return OFPERR_OFPBIC_BAD_LEN; \ > + } > +OVS_TABLE_FEATURE_PROPS > +#undef DEFINE_TFPROP > + > + default: > + return OFPERR_OFPTFFC_BAD_TYPE; > + } > +} > + > +static enum ofperr > +table_feature_check_len(const struct ofp13_table_features *feature) > +{ > + uint16_t len = ntohs(feature->length); > + > + if (len < sizeof(*feature) || len % 8) { sizeof is an operator, so you probably don't need the (). Likewise elsewhere in this patch. Perhaps you can use TFPROP_ALIGN or similar instead of 8? > + return OFPERR_OFPTFFC_BAD_LEN; > + } > + return 0; > +} > + > +static void > +table_feature_get_id(const struct ofp13_table_features *feature, > + uint8_t *table_id) > +{ > + *table_id = feature->table_id; > +} Can you just use feature->table_id directly and remove table_feature_get_id() altogether? > + > +static int > +table_feature_is_valid(const struct ofp13_table_features *feature, > + size_t n_feature) > +{ > + uint16_t len = ntohs(feature->length); > + return (!(len % 8) > + && len >= (sizeof *feature) > + && (len / sizeof *feature) <= n_feature); > +} > + > +static inline struct ofp13_table_features * > +table_feature_next(const struct ofp13_table_features *feature) > +{ > + return ((struct ofp13_table_features *) (void *) > + ((uint8_t *) feature + ntohs(feature->length))); > +} > + > +/* This macro is careful to check for props with bad lengths. */ > +#define TABLE_FEATURE_FOR_EACH(ITER, LEFT, FEATURES, N_FEATURES) \ > + for ((ITER) = (FEATURES), (LEFT) = (N_FEATURES); \ > + (LEFT) > 0 && table_feature_is_valid(ITER, LEFT); \ > + ((LEFT) -= (ntohs((ITER)->length) \ > + / MULTIPART_ALIGN), \ > + (ITER) = table_feature_next(ITER))) > + > +static enum ofperr > +decode_openflow13_table_features(const struct ofp13_table_features > features[], > + size_t n_features, > + const struct ofp13_table_features *out[]) > +{ > + const struct ofp13_table_features *feature; > + size_t left; > + > + memset(out, 0, OFTABLE_NUM * sizeof *out); > + TABLE_FEATURE_FOR_EACH (feature, left, features, n_features) { > + uint8_t table_id; > + enum ofperr error; > + > + error = table_feature_check_len(feature); > + if (error) { > + return error; > + } > + > + table_feature_get_id(feature, &table_id); > + > + if (out[table_id]) { > + /* CHECK if we could return a more meaningful type */ > + return OFPERR_OFPTFFC_BAD_TABLE; > + } > + out[table_id] = feature; > + } > + > + if (left) { > + VLOG_WARN( "Bad table features format at offset %zu.", > + (n_features - left) * sizeof *feature); > + return OFPERR_OFPBIC_BAD_LEN; > + } > + return 0; > +} > + > +static enum ofperr > +decode_openflow13_tfprops(const struct ofp13_table_feature_prop_header > props[], > + size_t n_props, > + const struct ofp13_table_feature_prop_header > *out[]) > +{ > + const struct ofp13_table_feature_prop_header *prop; > + size_t left; > + > + memset(out, 0, N_OVS_TFPROPS * sizeof *out); > + TFPROP_FOR_EACH (prop, left, props, n_props) { > + enum ofp13_table_feature_prop_type type; > + enum ofperr error; > + > + error = decode_openflow13_tfprop(prop, &type); > + if (error) { > + return error; > + } > + > + if (out[type]) { > + return OFPERR_OFPTFFC_BAD_TYPE; > + } > + out[type] = prop; > + } > + > + if (left) { > + VLOG_WARN( "Bad table features property format at offset %zu.", > + (n_props - left) * TFPROP_ALIGN); > + return OFPERR_OFPBIC_BAD_LEN; > + } > + return 0; > +} > + > +static size_t table_features_count_body_len(const struct ofp_header *oh) > +{ > + return (ntohs(oh->length) - sizeof(*oh) - MULTIPART_HDR_LEN); > +} > + > +static size_t > +table_features_count_props_len(const struct ofp13_table_features *feature) > +{ > + return ntohs(feature->length) - sizeof(*feature); > +} > + > +static const struct ofp13_table_feature_prop_header * > +table_feature_get_props(const struct ofp13_table_features *feature) > +{ > + return (const struct ofp13_table_feature_prop_header *) > + (feature + 1); > +} I think it would be nicer to just open-code the logic in the above three functions. It is rather simple and only used once. > + > +static enum ofperr > +translate_table_features(struct ofputil_table_features *tf, > + const struct ofp13_table_features *otf) > +{ > + enum ofperr error; > + const struct ofp13_table_feature_prop_header *props; > + uint16_t props_len; > + const struct ofp13_table_feature_prop_header *ps[N_OVS_TFPROPS]; > + int i; > + > + tf->length = ntohs(otf->length); > + tf->table_id = otf->table_id; > + ovs_strlcpy(tf->name, otf->name, OFP_MAX_TABLE_NAME_LEN); > + tf->metadata_match = ntohll(otf->metadata_match); > + tf->metadata_write = ntohll(otf->metadata_write); > + tf->config = ntohl(otf->config); > + tf->max_entries = ntohl(otf->max_entries); > + > + props = table_feature_get_props(otf); > + props_len = table_features_count_props_len(otf); > + error = decode_openflow13_tfprops(props, props_len / TFPROP_ALIGN, ps); > + if (error) { > + return error; > + } > + > + for (i = 0; i < N_OVS_TFPROPS; i++) { > + if (ps[i] == NULL) { > + continue; > + } > + trans_openflow13_tfprop(ps[i], &tf->props[i]); > + ++tf->n_property; > + } > + > + return 0; > +} > + > +enum ofperr > +ofputil_pull_table_features(const struct ofp_header *oh, int *tfs_num, > + struct ofputil_table_features tfs[], > + uint32_t *flag) > +{ > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); > + struct ofpbuf msg; > + enum ofpraw raw; > + > + const struct ofp13_table_features *features; > + const struct ofp13_table_features *fs[OFTABLE_NUM]; > + > + uint32_t features_len; > + int i; > + int tfs_cursor = 0; > + int error = 0; > + > + ofpbuf_use_const(&msg, oh, ntohs(oh->length)); > + raw = ofpraw_pull_assert(&msg); > + if (raw != OFPRAW_OFPST13_TABLE_FEATURES_REQUEST > + && raw != OFPRAW_OFPST13_TABLE_FEATURES_REPLY) { if (raw != OFPRAW_OFPST13_TABLE_FEATURES_REQUEST && raw != OFPRAW_OFPST13_TABLE_FEATURES_REPLY) { Can this ever occur? > + VLOG_DBG_RL(&rl, "Bad openflow msg type(%u).\n", raw); > + return OFPERR_OFPBRC_BAD_TYPE; > + } > + > + if (msg.size == 0) /* stands for GET table_features request */ { > + *flag = OVS_TF_GET; > + return 0; > + } > + > + /* Else if request contains body, then it's a SET table_features request > */ > + *flag = OVS_TF_SET; > + > + features_len = table_features_count_body_len(oh); > + features = ofpbuf_try_pull(&msg, features_len); > + if (features == NULL) { > + VLOG_WARN_RL(&rl, "Table features length %u is longer than space " > + "in message length %zu.", features_len, msg.size); > + return OFPERR_OFPTFFC_BAD_LEN; > + } > + > + error = decode_openflow13_table_features( > + features, features_len / MULTIPART_ALIGN, fs); > + if (error) { > + return error; > + } > + > + for (i = 0; i < OFTABLE_NUM; i++) { > + if (fs[i] == NULL) { > + continue; > + } > + > + translate_table_features(&tfs[tfs_cursor++], fs[i]); > + } > + > + *tfs_num = tfs_cursor; > + return error; > +} > + > +static enum ofperr > +ofputil_encode_table_features_props(struct ofpbuf *reply, > + const struct ofputil_table_feature_prop_header > *props, > + int prop_num) > +{ > + int i; > + enum ofperr error; > + > + for (i = 0; i < prop_num; i++) { > + const struct ofputil_table_feature_prop_header *prop = &props[i]; > + if (!prop->data || !prop->length) { > + continue; > + } > + > + error = encode_openflow13_tfprop(reply, prop); > + if (error) { > + return error; > + } > + } > + > + return 0; > +} > + > +void > +ofputil_append_table_features_reply(const struct ofputil_table_features *tf, > + struct list *replies) > +{ > + struct ofpbuf *reply = ofpbuf_from_list(list_back(replies)); > + size_t start_otf = reply->size; > + enum ofpraw raw; > + > + ofpraw_decode_partial(&raw, reply->data, reply->size); > + if (raw == OFPRAW_OFPST13_TABLE_FEATURES_REPLY) { > + struct ofp13_table_features *otf; > + > + ofpbuf_put_zeros(reply, sizeof *otf); > + ofputil_encode_table_features_props(reply, tf->props, > tf->n_property); > + > + otf = ofpbuf_at_assert(reply, start_otf, sizeof *otf); > + otf->length = htons(reply->size - start_otf); > + otf->table_id = tf->table_id; > + ovs_strlcpy(otf->name, tf->name, OFP_MAX_TABLE_NAME_LEN); > + otf->metadata_match = htonll(tf->metadata_match); > + otf->metadata_write = htonll(tf->metadata_write); > + otf->config = htonl(tf->config); > + otf->max_entries = htonl(tf->max_entries); > + } else { > + NOT_REACHED(); > + } > + ofpmp_postappend(replies, start_otf); > +} > + > +/* Returns an OpenFlow group features request for OpenFlow version > + * 'ofp_version'. */ > +struct ofpbuf * > +ofputil_encode_table_features_request(enum ofp_version ofp_version) > +{ > + struct ofpbuf *request = NULL; > + > + switch (ofp_version) { > + case OFP10_VERSION: > + case OFP11_VERSION: > + ovs_fatal(0, "dump-table-features needs OpenFlow 1.2 or later " > + "(\'-O OpenFlow12\')"); > + case OFP12_VERSION: > + case OFP13_VERSION: { > + request = ofpraw_alloc(OFPRAW_OFPST13_TABLE_FEATURES_REQUEST, > + ofp_version, 0); > + break; > + } > + default: > + NOT_REACHED(); > + } > + > + return request; > +} > + > /* ofputil_table_mod */ > > /* Decodes the OpenFlow "table mod" message in '*oh' into an abstract form in > diff --git a/lib/ofp-util.h b/lib/ofp-util.h > index fef85e0..a5bef77 100644 > --- a/lib/ofp-util.h > +++ b/lib/ofp-util.h > @@ -599,6 +599,159 @@ enum ofperr ofputil_decode_table_mod(const struct > ofp_header *, > struct ofpbuf *ofputil_encode_table_mod(const struct ofputil_table_mod *, > enum ofputil_protocol); > > +struct ofputil_table_feature_prop_header { > + uint16_t type; /* One of OFPTFPT_*. */ > + uint16_t length; /* Length in bytes of this property. */ > + void *data; > +}; > + > + > +enum ovs_table_features_flag { > + OVS_TF_GET = (1 << 0), > + OVS_TF_SET = (1 << 1) > +}; > + > +#define OFTABLE_NUM 0xff > + > +/* Abstract ofp13_table_features */ > +struct ofputil_table_features { > + uint16_t length; /* Length is padded to 64 bits. */ > + uint8_t table_id; /* Identifier of table. Lower numbered tables > + are consulted first. */ > + char name[OFP_MAX_TABLE_NAME_LEN]; > + uint64_t metadata_match; /* Bits of metadata table can match. */ > + uint64_t metadata_write; /* Bits of metadata table can write. */ > + uint32_t config; /* Bitmap of OFPTC_* values */ > + uint32_t max_entries; /* Max number of entries supported. */ > + > + struct ofputil_table_feature_prop_header props[0xff]; > + uint16_t n_property; > +}; > + > +struct oxm_info { > + uint32_t type; > + char *name; > +}; > + > +extern struct oxm_info oxm_info[]; > +int get_oxm_num(void); > +char *get_oxm_name(uint32_t type); > + > +/* Table Feature property types. > + * Low order bit cleared indicates a property for a regular Flow Entry. > + * Low order bit set indicates a property for the Table-Miss Flow Entry. */ > +enum ofputil_table_feature_prop_type { > + OFPUTIL_INSTRUCTIONS = 0, /* Instructions property. */ > + OFPUTIL_INSTRUCTIONS_MISS = 1, /* Instructions for table-miss. */ > + OFPUTIL_NEXT_TABLES = 2, /* Next Table property. */ > + OFPUTIL_NEXT_TABLES_MISS = 3, /* Next Table for table-miss. */ > + OFPUTIL_WRITE_ACTIONS = 4, /* Write Actions property. */ > + OFPUTIL_WRITE_ACTIONS_MISS = 5, /* Write Actions for table-miss. */ > + OFPUTIL_APPLY_ACTIONS = 6, /* Apply Actions property. */ > + OFPUTIL_APPLY_ACTIONS_MISS = 7, /* Apply Actions for table-miss. */ > + OFPUTIL_MATCH = 8, /* Match property. */ > + OFPUTIL_WILDCARDS = 10, /* Wildcards property. */ > + OFPUTIL_WRITE_SETFIELD = 12, /* Write Set-Field property. */ > + OFPUTIL_WRITE_SETFIELD_MISS = 13, /* Write Set-Field for table-miss. */ > + OFPUTIL_APPLY_SETFIELD = 14, /* Apply Set-Field property. */ > + OFPUTIL_APPLY_SETFIELD_MISS = 15, /* Apply Set-Field for table-miss. */ > + OFPUTIL_EXPERIMENTER = 0xFFFE, /* Experimenter property. */ > + OFPUTIL_EXPERIMENTER_MISS = 0xFFFF, /* Experimenter for table-miss. */ > +}; I'm still not sure why this is needed when enum ofp13_table_feature_prop_type already exists. > + > +struct ofputil_instruction { > + uint16_t type; /* Instruction type */ Perhaps the type of the 'type' field could be enum ofp13_table_feature_prop_type? Likewise several times below. > + uint16_t len; /* Length of this struct in bytes. */ > + uint8_t pad[4]; > +}; > + > +/* Instructions property */ > +struct ofputil_table_feature_prop_instructions { > + uint16_t type; /* One of OFPUTIL_INSTRUCTIONS, > + OFPUTIL_INSTRUCTIONS_MISS. */ > + uint16_t length; /* Length in bytes of this property. */ > + uint8_t table_id; > + uint8_t n_instructions; > + /* Followed by: > + * - Exactly (length - 4) bytes containing the instruction ids, then > + * - Exactly (length + 7)/8*8 - (length) (between 0 and 7) > + * bytes of all-zero bytes */ > + struct ofputil_instruction *instruction_ids; > +}; > + > +#define NEXT_TABLE_NUM 0xff > + > +struct ofputil_table_feature_prop_next_tables { > + uint16_t type; /* One of OFPUTIL_NEXT_TABLES, > + OFPUTIL_NEXT_TABLES_MISS. */ > + uint16_t length; /* Length in bytes of this property. */ > + uint8_t n_next_table; > + /* Followed by: > + * - Exactly (length - 4) bytes containing the table_ids, then > + * - Exactly (length + 7)/8*8 - (length) (between 0 and 7) > + * bytes of all-zero bytes */ > + uint8_t next_table_ids[NEXT_TABLE_NUM]; /* List of table ids. */ > +}; > + > +struct ofputil_table_feature_prop_actions { > + uint16_t type; /* One of OFPUTIL_WRITE_ACTIONS, > + OFPUTIL_WRITE_ACTIONS_MISS, > + OFPUTIL_APPLY_ACTIONS, > + OFPUTIL_APPLY_ACTIONS_MISS. */ > + uint16_t length; /* Length in bytes of this property. */ > + uint8_t n_actions; > + /* Followed by: > + * - Exactly (length - 4) bytes containing the action_ids, then > + * - Exactly (length + 7)/8*8 - (length) (between 0 and 7) > + * bytes of all-zero bytes */ > + struct ofp_action_header *action_ids; /* List of actions > + without any data */ > +}; > + > +/* Match, Wildcard or Set-Field property */ > +struct ofputil_table_feature_prop_oxm { > + uint16_t type; /* One of OFPTFPT13_MATCH, OFPTFPT13_WILDCARDS, > + OFPTFPT13_WRITE_SETFIELD, > + OFPTFPT13_WRITE_SETFIELD_MISS, > + OFPTFPT13_APPLY_SETFIELD, > + OFPTFPT13_APPLY_SETFIELD_MISS. */ > + uint16_t length; /* Length in bytes of this property. */ > + uint8_t n_oxm; > + /* Followed by: > + * - Exactly (length - 4) bytes containing the oxm_ids, then > + * - Exactly (length + 7)/8*8 - (length) (between 0 and 7) > + * bytes of all-zero bytes */ > + /* enum ofputil_match_bitmap oxm_bits; */ /* Array of OXM headers */ > +}; > + > +/* Experimenter table feature property */ > +struct ofputil_table_feature_prop_experimenter { > + uint16_t type; /* One of OFPTFPT13_EXPERIMENTER, > + OFPTFPT13_EXPERIMENTER_MISS. */ > + uint16_t length; /* Length in bytes of this property. */ > + uint32_t experimenter; /* Experimenter ID which takes the same form > + as in struct ofp_experimenter_header. */ > + uint32_t exp_type; /* Experimenter defined. */ > + /* Followed by: > + * - Exactly (length - 12) bytes containing the experimenter data, then > + * - Exactly (length + 7)/8*8 - (length) (between 0 and 7) > + * bytes of all-zero bytes */ > + /*uint32_t experimenter_data[10]; */ > +}; > + > +enum ofperr > +ofputil_pull_table_features(const struct ofp_header *oh, int *n, > + struct ofputil_table_features tfs[], > + uint32_t *flag); > +struct ofpbuf * > +ofputil_encode_table_features_request(enum ofp_version ofp_version); > +void > +ofputil_append_table_features_reply(const struct ofputil_table_features *tf, > + struct list *replies); > + > +uint16_t table_feature_prop_get_length(uint16_t type); > +char *table_feature_prop_get_name(uint16_t type); > + > /* Meter band configuration for all supported band types. */ > struct ofputil_meter_band { > uint16_t type; > -- > 1.7.3.1.msysgit.0 > > _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev