[ovs-dev] [PATCH v2 2/8] ofp-util: Implement OFPMP_TABLE_FEATURES en/decode
Simon Horman
horms at verge.net.au
Wed Nov 13 08:21:44 UTC 2013
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.wu at 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
>
>
More information about the dev
mailing list