[ovs-dev] [PATCH v2 2/8] ofp-util: Implement OFPMP_TABLE_FEATURES en/decode
Alexander Wu
alexander.wu at huawei.com
Thu Nov 21 09:39:28 UTC 2013
On 13/11/2013 16:21, Simon Horman wrote:
> 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.
>
Thanks, fixed.
>> +{
>> + 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.
>
Thanks. The struct is removed.
>> + 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.
>
Thanks, removed.
>> + 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.
>
Thanks, fixed.
>> +{
>> + 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.
>
Thanks, typeof type fixed, but the return type uint16_t is not changed because
the length of all table feature properties use uint16_t. The length guarantees
all struct here won't be bigger than uint16_t.
>> +{
>> + 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?
>
Thanks, it's useless, I remove it.
>> + && 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?
>
Not sure. But "action_next" and "instruction_next" do like this.
Is it trying to avoid compilier warning?
>> + ((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?
>
I think the first "+" is a unary operator, standing for positive number.
>> +
>> +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.
>
Yeah. Comment removed.
>> + prop->data = xmalloc(data_len);
>> + memcpy(prop->data, (void *)(oprop + 1), data_len);
>
> You can use OPROP_DATA() in the line above.
>
Thanks, fixed.
>> + 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) {
>
Thanks, fixed.
>> + 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.
>
Thanks, not changed. I'm thinking about way to translate them
more general.
>> +
>> + 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.
>
Thanks, removed.
>> + 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?
>
Thanks, the former fixed, the latter not. I'll fix it soon.
>> + 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?
>
For further steps, the table_id may be removed from feature, and
use the offset of ofproto->oftable[i] directly. So I reserve this
function.
>> +
>> +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.
>
Thanks, fixed.
>> +
>> +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?
>
Yes. I reuse the function to decode table-feature at ofp-print.
Maybe it's ambiguous and not good to understand.
>> + 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