[ovs-dev] [PATCH 2/4] Implement the encode/decode Table Features functions
Simon Horman
horms at verge.net.au
Fri Nov 1 04:29:26 UTC 2013
On Sat, Oct 26, 2013 at 06:14:28PM +0800, Alexander Wu wrote:
> 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-print.c | 128 +++++++++++-
> lib/ofp-util.c | 646 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 772 insertions(+), 2 deletions(-)
>
> diff --git a/lib/ofp-print.c b/lib/ofp-print.c
> index e4d0303..418f918 100644
> --- a/lib/ofp-print.c
> +++ b/lib/ofp-print.c
> @@ -2381,6 +2381,127 @@ ofp_print_group_mod(struct ds *s, const struct ofp_header *oh)
> ofp_print_group(s, gm.group_id, gm.type, &gm.buckets);
> }
>
> +/* Appends a string representation of 'prop' to 's'. */
> +static void
> +table_feature_prop_format(const struct ofputil_table_feature_prop_header *prop,
> + struct ds *s)
static void
table_feature_prop_format(const struct ofputil_table_feature_prop_header *prop,
struct ds *s)
> +{
> + int i = 0;
> + int n = 0;
> + int element_size = (int)get_prop_length(prop->type);
Blank line here please.
> + if (!element_size) {
> + /* FIXME LOG SOMETHING */
This should be fixed. I can think of three ways:
* Please add a (rate limited?) log message here
* Return an error and handle it in the callers
* NOT_REACHED(), though in that case you could remove the check
all together as a division by zero should alert the user that
there is a bug somewhere.
The last option seems easiest. Is it a bug for element_size to be zero?
> + return;
> + } else {
> + n = (prop->length - 4) / element_size;
> + }
> +
> + ds_put_format(s, "%s: ", get_prop_name(prop->type));
> +
> + switch (prop->type) {
> + case OFPTFPT13_INSTRUCTIONS:
> + case OFPTFPT13_INSTRUCTIONS_MISS: {
> + struct ofp11_instruction *inst = (struct ofp11_instruction *)prop->data;
> +
> + /* FIXME ofpacts_format */
Is the comment above still valid?
If so, could you fix it?
> + for (i = 0; i < n; i++) {
> + ds_put_format(s, "%"PRIu16, inst[i].type);
> + if (i != n - 1)
> + ds_put_format(s, ",");
> + }
> + break;
> + }
> + case OFPTFPT13_NEXT_TABLES:
> + case OFPTFPT13_NEXT_TABLES_MISS: {
> + uint8_t *ntables = prop->data;
> + for (i = 0; i < n; i++) {
> + ds_put_format(s, "%"PRIu8, ntables[i]);
> + if (i != n - 1)
> + ds_put_format(s, ",");
> + }
> + break;
> + }
> + case OFPTFPT13_WRITE_ACTIONS:
> + case OFPTFPT13_WRITE_ACTIONS_MISS:
> + case OFPTFPT13_APPLY_ACTIONS:
> + case OFPTFPT13_APPLY_ACTIONS_MISS: {
> + struct ofp_action_header *acts =(struct ofp_action_header *)prop->data;
> +
> + /* FIXME ofpacts_format */
Ditto.
> + for (i = 0; i < n; i++) {
> + ds_put_format(s, "%"PRIu16, acts[i].type);
> + if (i != n - 1)
> + ds_put_format(s, ",");
> + }
> + break;
> + }
> + case OFPTFPT13_MATCH:
> + case OFPTFPT13_WILDCARDS:
> + case OFPTFPT13_WRITE_SETFIELD:
> + case OFPTFPT13_WRITE_SETFIELD_MISS:
> + case OFPTFPT13_APPLY_SETFIELD:
> + case OFPTFPT13_APPLY_SETFIELD_MISS: {
> + uint32_t *oxm = (uint32_t *)prop->data;
> +
> + for (i = 0; i < n; i++) {
> + ds_put_format(s, "%s", get_oxm_name(oxm[i]));
> + if (i != n - 1)
> + ds_put_format(s, ",");
> + }
> + break;
> + }
> + case OFPTFPT13_EXPERIMENTER:
> + case OFPTFPT13_EXPERIMENTER_MISS:
> + ds_put_format(s, "experimenter");
> + if (i != n - 1)
> + ds_put_format(s, ",");
> + break;
> + default:
> + ds_put_format(s, "unknown(%u)", prop->type);
> + if (i != n - 1)
> + ds_put_format(s, ",");
> + break;
> + }
> +}
> +
> +
> +static void
> +ofp_print_table_features_stats_single(struct ds *s,
> + const struct ofputil_table_features *tf)
> +{
> + int i;
> + ds_put_format(s, "\n %"PRIu8":", tf->table_id);
> + ds_put_format(s, " name:%s", tf->name);
> + ds_put_format(s, " metadata_match:%"PRIx64, tf->metadata_match);
> + ds_put_format(s, " metadata_write:%"PRIx64, tf->metadata_write);
> + ds_put_format(s, " config:%"PRIx32, tf->config);
> + ds_put_format(s, " max_entries:%"PRIu32, tf->max_entries);
> +
> + ds_put_format(s, "\n Properties:");
> + for (i = 0; i < tf->n_property; i++) {
> + if (tf->props[i].data == NULL || tf->props[i].length == 0)
> + continue;
> +
> + ds_put_format(s, "\n ");
> + table_feature_prop_format(&tf->props[i], s);
> + }
> + ds_put_format(s, "\n");
> +}
> +
> +static void
> +ofp_print_table_features_stats(struct ds *s, const struct ofp_header *oh)
> +{
> + struct ofputil_table_features tfs[0xff + 1];
> + int tfs_num;
> + int i;
> +
> + ofputil_decode_table_features_reply(oh, &tfs_num, tfs);
> +
> + for (i = 0; i < tfs_num; i++) {
> + ofp_print_table_features_stats_single(s, &tfs[i]);
> + }
> +}
> +
> static void
> ofp_to_string__(const struct ofp_header *oh, enum ofpraw raw,
> struct ds *string, int verbosity)
> @@ -2419,10 +2540,13 @@ ofp_to_string__(const struct ofp_header *oh, enum ofpraw raw,
> ofp_print_group_mod(string, oh);
> break;
>
> - case OFPTYPE_QUEUE_GET_CONFIG_REQUEST:
> - case OFPTYPE_QUEUE_GET_CONFIG_REPLY:
> case OFPTYPE_TABLE_FEATURES_STATS_REQUEST:
> case OFPTYPE_TABLE_FEATURES_STATS_REPLY:
> + ofp_print_table_features_stats(string, oh);
> + break;
> +
> + case OFPTYPE_QUEUE_GET_CONFIG_REQUEST:
> + case OFPTYPE_QUEUE_GET_CONFIG_REPLY:
> ofp_print_not_implemented(string);
> break;
>
> diff --git a/lib/ofp-util.c b/lib/ofp-util.c
> index 8c200ce..090e0a4 100644
> --- a/lib/ofp-util.c
> +++ b/lib/ofp-util.c
> @@ -3752,6 +3752,652 @@ ofputil_encode_port_mod(const struct ofputil_port_mod *pm,
> return b;
> }
>
> +static enum ofperr table_features_move_data(uint8_t *dst, uint8_t **src,
> + uint32_t *length, uint32_t data_len)
static enum ofperr
table_features_move_data(uint8_t *dst, uint8_t **src, uint32_t *length,
uint32_t data_len)
Perhaps void * would be a more convenient type for dst.
> +{
> + memcpy(dst, *src, data_len);
> + if (*length < data_len)
> + return OFPERR_OFPTFFC_BAD_LEN;
Should this check occur before the memcpy?
> + *length -= data_len;
> + *src += data_len;
> + return 0;
> +}
> +
> +static enum ofperr
> +decode_table_features_prop_header(uint8_t **p, uint32_t *length,
> + struct ofputil_table_feature_prop_header *prop)
> +{
> + struct ofp13_table_feature_prop_header oprop;
> +
> + table_features_move_data((uint8_t*)&oprop, p, length, sizeof(oprop));
> +
> + prop->type = ntohs(oprop.type);
> + prop->length = ntohs(oprop.length);
> +
> + if (prop->length < sizeof(oprop)) {
> + VLOG_DBG("decode table feature property err: prop length %u < "
> + "min header length %zu \n", prop->length, sizeof(oprop));
Perhaps this logging should be rate-limited?
> + return OFPERR_OFPTFFC_BAD_LEN;
> + }
> +
> + return 0;
> +}
> +
> +static int prop_get_n_elem(uint32_t *n_elem, uint16_t length, uint16_t elem_size)
static int
prop_get_n_elem(uint32_t *n_elem, uint16_t length, uint16_t elem_size)
> +{
> + int n = 0;
> + if (length % elem_size)
> + return OFPERR_OFPTFFC_BAD_LEN;
if (length % elem_size) {
return OFPERR_OFPTFFC_BAD_LEN;
}
> +
> + n = length / elem_size;
> + *n_elem = n;
> +
> + return 0;
> +}
> +
> +static void ntoh_instruction_array(uint8_t *array, uint16_t n_elem)
> +{
> + int i;
> + struct ofp11_instruction *oi = (struct ofp11_instruction *)array;
> + for (i = 0; i < n_elem; i++) {
> + oi[i].len = ntohs(oi[i].len);
> + oi[i].type = ntohs(oi[i].type);
> + }
> +}
I am quite uncomfortable about this and I believe that sparse would be too.
The type of the 'len' and 'type' fields is ovs_be16, a big-endiean type.
But you are saving host-endian values there. So a user of ofp11_instruction
now needs to know which way to read the values which seems error
prone and excludes the use of static analysis to detect bugs.
Can you make use of struct ofputil_instruction to save the values
in a type-safe manner?
As an aside. I run sparse using the following:
MAKE=make make clean
MAKE=make make C=2 CF="-D__CHECK_ENDIAN__ -Wsparse-all" all > /dev/null
And I use the 30cb3a104d4fb8b704 ("Get rid of gcc warning about enum
values") revision of git://git.kernel.org/pub/scm/devel/sparse/sparse.git.
I believe that at least at some point in time that was consistent with
some of the Nicira developers.
> +static void ntoh_action_array(uint8_t *array, uint16_t n_elem)
> +{
> + int i;
> + struct ofp_action_header *oa = (struct ofp_action_header *)array;
> + for (i = 0; i < n_elem; i++) {
> + oa[i].len = ntohs(oa[i].len);
> + oa[i].type = ntohs(oa[i].type);
> + }
> +}
Ditto.
In this case perhaps you need to create
ofputil_action_header with host-endian 'len' and 'type' fields.
> +
> +static void ntoh_be32_array(uint8_t *array, uint16_t n_elem)
> +{
> + int i;
> + ovs_be32 *be32 = (ovs_be32 *)array;
> + for (i = 0; i < n_elem; i++) {
> + be32[i] = ntohl(be32[i]);
> + }
> +}
Ditto.
In this case I think you should supply an array of ovs_be32 to
save the values into.
> +
> +static void hton_instruction_array(uint8_t *array, uint16_t n_elem)
> +{
> + int i;
> + struct ofp11_instruction *oi = (struct ofp11_instruction *)array;
> + for (i = 0; i < n_elem; i++) {
> + oi[i].len = htons(oi[i].len);
> + oi[i].type = htons(oi[i].type);
> + }
> +}
Ditto, except everything is reversed.
> +
> +static void hton_action_array(uint8_t *array, uint16_t n_elem)
> +{
> + int i;
> + struct ofp_action_header *oa = (struct ofp_action_header *)array;
> + for (i = 0; i < n_elem; i++) {
> + oa[i].len = htons(oa[i].len);
> + oa[i].type = htons(oa[i].type);
> + }
> +}
Ditto.
> +
> +static void hton_be32_array(uint8_t *array, uint16_t n_elem)
> +{
> + int i;
> + ovs_be32 *be32 = (ovs_be32 *)array;
> + for (i = 0; i < n_elem; i++) {
> + be32[i] = htonl(be32[i]);
> + }
> +}
Ditto.
> +
> +struct oxm_variable oxm_variables[] = {
> + {OXM_OF_IN_PORT, "IN_PORT"},
> + {OXM_OF_IN_PHY_PORT, "IN_PHY_PORT"},
> + {OXM_OF_METADATA, "METADATA"},
> + {OXM_OF_ETH_DST, "ETH_DST"},
> + {OXM_OF_ETH_SRC, "ETH_SRC"},
> + {OXM_OF_ETH_TYPE, "ETH_TYPE"},
> + {OXM_OF_VLAN_VID, "VLAN_VID"},
> + {OXM_OF_VLAN_PCP, "VLAN_PCP"},
> + {OXM_OF_IP_DSCP, "IP_DSCP"},
> + {OXM_OF_IP_ECN, "IP_ECN"},
> + {OXM_OF_IP_PROTO, "IP_PROTO"},
> + {OXM_OF_IPV4_SRC, "IPV4_SRC"},
> + {OXM_OF_IPV4_DST, "IPV4_DST"},
> + {OXM_OF_TCP_SRC, "TCP_SRC"},
> + {OXM_OF_TCP_DST, "TCP_DST"},
> + {OXM_OF_UDP_SRC, "UDP_SRC"},
> + {OXM_OF_UDP_DST, "UDP_DST"},
> + {OXM_OF_SCTP_SRC, "SCTP_SRC"},
> + {OXM_OF_SCTP_DST, "SCTP_DST"},
> + {OXM_OF_ICMPV4_TYPE, "ICMPV4_TYPE"},
> + {OXM_OF_ICMPV4_CODE, "ICMPV4_CODE"},
> + {OXM_OF_ARP_OP, "ARP_OP"},
> + {OXM_OF_ARP_SPA, "ARP_SPA"},
> + {OXM_OF_ARP_TPA, "ARP_TPA"},
> + {OXM_OF_ARP_SHA, "ARP_SHA"},
> + {OXM_OF_ARP_THA, "ARP_THA"},
> + {OXM_OF_IPV6_SRC, "IPV6_SRC"},
> + {OXM_OF_IPV6_DST, "IPV6_DST"},
> + {OXM_OF_IPV6_FLABEL, "IPV6_FLABEL"},
> + {OXM_OF_ICMPV6_TYPE, "ICMPV6_TYPE"},
> + {OXM_OF_ICMPV6_CODE, "ICMPV6_CODE"},
> + {OXM_OF_IPV6_ND_TARGET, "IPV6_ND_TARGET"},
> + {OXM_OF_IPV6_ND_SLL, "IPV6_ND_SLL"},
> + {OXM_OF_IPV6_ND_TLL, "IPV6_ND_TLL"},
> + {OXM_OF_MPLS_LABEL, "MPLS_LABEL"},
> + {OXM_OF_MPLS_TC, "MPLS_TC"},
> + {OXM_OF_MPLS_BOS, "MPLS_BOS"},
> + {OXM_OF_TUNNEL_ID, "TUNNEL_ID"},
> + {OXM_OF_IPV6_EXTHDR, "IPV6_EXTHDR"},
> +};
> +
> +int get_oxm_num(void)
> +{
> + return ARRAY_SIZE(oxm_variables);
> +}
> +
> +char *get_oxm_name(uint32_t type)
> +{
> + int i;
> + int n = ARRAY_SIZE(oxm_variables);
Perhaps get_oxm_num() should be used here?
Also, a blank line would be nice here.
Likewise many times below.
> + for (i = 0; i < n; i++) {
> + if (type == oxm_variables[i].data)
> + return oxm_variables[i].name;
> + }
> + return NULL;
> +}
> +
> +struct table_feature_prop {
> + uint16_t type;
> + uint16_t length;
> + void (*array_ntoh)(uint8_t*, uint16_t);
> + void (*array_hton)(uint8_t*, uint16_t);
These function pointers seem really hairy to me.
I think it would be cleaner to just have case statements
in the callers. For starters it would remove the need
to cast the sources to uint8_t *, thus loosing the
rudimentary type-safety C provides on the way.
> + char *name;
> +};
> +
> +static struct table_feature_prop static_props[] = {
> + {OFPTFPT13_INSTRUCTIONS, sizeof(struct ofp11_instruction),
> + ntoh_instruction_array, hton_instruction_array, "OFPTFPT13_INSTRUCTIONS"},
> + {OFPTFPT13_INSTRUCTIONS_MISS, sizeof(struct ofp11_instruction),
> + ntoh_instruction_array, hton_instruction_array, "OFPTFPT13_INSTRUCTIONS_MISS"},
> + {OFPTFPT13_NEXT_TABLES, sizeof(uint8_t), NULL, NULL, "OFPTFPT13_NEXT_TABLES"},
> + {OFPTFPT13_NEXT_TABLES_MISS, sizeof(uint8_t), NULL, NULL, "OFPTFPT13_NEXT_TABLES_MISS"},
Some of the lines above are greater than 80 columns wide.
> + {OFPTFPT13_WRITE_ACTIONS, sizeof(struct ofp_action_header),
> + ntoh_action_array, hton_action_array, "OFPTFPT13_WRITE_ACTIONS"},
> + {OFPTFPT13_WRITE_ACTIONS_MISS, sizeof(struct ofp_action_header),
> + ntoh_action_array, hton_action_array, "OFPTFPT13_WRITE_ACTIONS_MISS"},
> + {OFPTFPT13_APPLY_ACTIONS, sizeof(struct ofp_action_header),
> + ntoh_action_array, hton_action_array, "OFPTFPT13_APPLY_ACTIONS"},
> + {OFPTFPT13_APPLY_ACTIONS_MISS, sizeof(struct ofp_action_header),
> + ntoh_action_array, hton_action_array, "OFPTFPT13_APPLY_ACTIONS_MISS"},
> + {OFPTFPT13_MATCH, sizeof(ovs_be32),
> + ntoh_be32_array, hton_be32_array, "OFPTFPT13_MATCH"},
> + {OFPTFPT13_WILDCARDS, sizeof(ovs_be32),
> + ntoh_be32_array, hton_be32_array, "OFPTFPT13_WILDCARDS"},
> + {OFPTFPT13_WRITE_SETFIELD, sizeof(ovs_be32),
> + ntoh_be32_array, hton_be32_array, "OFPTFPT13_WRITE_SETFIELD"},
> + {OFPTFPT13_WRITE_SETFIELD_MISS, sizeof(ovs_be32),
> + ntoh_be32_array, hton_be32_array, "OFPTFPT13_WRITE_SETFIELD_MISS"},
> + {OFPTFPT13_APPLY_SETFIELD, sizeof(ovs_be32),
> + ntoh_be32_array, hton_be32_array, "OFPTFPT13_APPLY_SETFIELD"},
> + {OFPTFPT13_APPLY_SETFIELD_MISS, sizeof(ovs_be32),
> + ntoh_be32_array, hton_be32_array, "OFPTFPT13_APPLY_SETFIELD_MISS"},
> + {OFPTFPT13_EXPERIMENTER, sizeof(ovs_be32),
> + ntoh_be32_array, hton_be32_array, "OFPTFPT13_EXPERIMENTER"},
> + {OFPTFPT13_EXPERIMENTER_MISS, sizeof(ovs_be32),
> + ntoh_be32_array, hton_be32_array, "OFPTFPT13_EXPERIMENTER_MISS"},
> +};
> +
> +/* CHECK to REPLACE if necessary */
What does this comment mean?
> +char *get_prop_name(uint16_t type)
> +{
> + int i;
> + int n = ARRAY_SIZE(static_props);
> + for (i = 0; i < n; i++) {
> + if (static_props[i].type == type) {
> + return static_props[i].name;
> + }
> + }
> + return NULL;
> +}
> +
> +/* CHECK to REPLACE if necessary */
> +uint16_t get_prop_length(uint16_t type)
> +{
> + int i;
> + int n = ARRAY_SIZE(static_props);
> + for (i = 0; i < n; i++) {
> + if (static_props[i].type == type) {
> + return static_props[i].length;
> + }
> + }
> + return 0;
> +}
> +
> +/* CHECK to REPLACE if necessary */
> +static void *get_prop_array_ntoh_func(uint16_t type)
I think you should tighten up the return type of this function.
Perhaps a typedef of a pointer to a function with the appropriate
return type and parameters?
> +{
> + int i;
> + int n = ARRAY_SIZE(static_props);
> + for (i = 0; i < n; i++) {
> + if (static_props[i].type == type) {
> + return static_props[i].array_ntoh;
> + }
> + }
> + return NULL;
> +}
> +
> +/* CHECK to REPLACE if necessary */
> +static void *get_prop_array_hton_func(uint16_t type)
Ditto
> +{
> + int i;
> + int n = ARRAY_SIZE(static_props);
> + for (i = 0; i < n; i++) {
> + if (static_props[i].type == type) {
> + return static_props[i].array_hton;
> + }
> + }
> + return NULL;
> +}
> +
> +static enum ofperr prop_data_trans(uint16_t type, uint32_t length, uint8_t *data, bool ntoh)
static enum ofperr
prop_data_trans(uint16_t type, uint32_t length, uint8_t *data, bool ntoh)
As I have mentioned above, I am not really very enthusiastic about this
approach. In particular because it involves storing both host and
big endian values in the same variables, breaking any hope of static
analysis helping us if the caller messes things up.
> +{
> + enum ofperr error = 0;
> + uint32_t data_len = 0;
> + uint32_t element_size = 0;
> + uint32_t n_elem = 0;
> +
> + void (*trans_array)(uint8_t *, uint16_t);
> +
> + data_len = length - sizeof(struct ofp13_table_feature_prop_header);
> + element_size = get_prop_length(type);
> + if (0 == element_size) {
> + return OFPERR_OFPTFFC_BAD_TYPE;
> + } else if (data_len % element_size) {
> + return OFPERR_OFPTFFC_BAD_LEN;
> + }
> +
> + error = prop_get_n_elem(&n_elem, data_len, element_size);
> + if (error)
> + return error;
> +
> + if (ntoh)
> + trans_array = get_prop_array_ntoh_func(type);
> + else
> + trans_array = get_prop_array_hton_func(type);
> +
> + if (trans_array)
> + trans_array(data, n_elem);
> +
> + return 0;
> +}
> +
> +static enum ofperr prop_data_ntoh(uint16_t type, uint32_t length, uint8_t *data)
static enum ofperr
prop_data_ntoh(uint16_t type, uint32_t length, uint8_t *data)
> +{
> + return prop_data_trans(type, length, data, true);
> +}
> +
> +static enum ofperr prop_data_hton(uint16_t type, uint32_t length, uint8_t *data)
static enum ofperr
prop_data_hton(uint16_t type, uint32_t length, uint8_t *data)
> +{
> + return prop_data_trans(type, length, data, false);
> +}
> +
> +static enum ofperr
> +decode_table_feature_prop(uint8_t **p, uint32_t *length,
> + struct ofputil_table_feature_prop_header *prop)
+static enum ofperr
decode_table_feature_prop(uint8_t **p, uint32_t *length,
struct ofputil_table_feature_prop_header *prop)
> +{
> + enum ofperr error = 0;
> + uint32_t data_len = 0;
> + uint32_t padding_len = 0;
> +
> + error = decode_table_features_prop_header(p, length, prop);
> + if (error)
> + return error;
> +
> + data_len = prop->length - sizeof(struct ofp13_table_feature_prop_header);
I think the following is slightly nicer as it is resistant to
prop changing type.
data_len = prop->length - sizeof(*prop);
> +
> + /* NOTE SPECIAL XMALLOC HERE FIXME */
> + prop->data = xmalloc(data_len);
> + table_features_move_data(prop->data, p, length, data_len);
> +
> + padding_len = ROUND_UP(prop->length, 8) - prop->length;
What is 8?
Can it be a #define or similar?
I see this calculation is made more than once.
Could it factored out into a function?
> + if (padding_len) {
> + *p += padding_len;
> + *length -= padding_len;
> + }
> +
> + if ((error = prop_data_ntoh(prop->type, prop->length, prop->data)))
> + return error;
> +
> + return 0;
> +}
> +
> +static enum ofperr
> +decode_table_feature_props(uint8_t **p, uint32_t length,
> + struct ofputil_table_features *tf)
> +{
> + enum ofperr error = 0;
> + int i = 0;
> +
> + while (length > 0) {
> + error = decode_table_feature_prop(p, &length, &tf->props[i]);
> + if (error)
> + return error;
> + ++i;
> + }
> +
> + tf->n_property = i;
> + return 0;
> +}
> +
> +
> +static enum ofperr
> +decode_table_feature_raw(uint8_t **p, uint32_t *length,
> + struct ofputil_table_features *tf)
> +{
> + struct ofp13_table_features otf;
> + uint32_t props_len;
> + int error = 0;
> +
> + if (*length == 0) {
> + /* do nothing if there is no body */
> + goto out;
As there is no cleanup associated with the out label I think
it is to remove it and return directly as necessary.
> + } else if (*length < sizeof(otf)) {
> + VLOG_DBG("Table features decode bad length, "
> + "length(%u) < min size(%zu).\n", *length, sizeof(otf));
Should this logging be rate limited?
> + return OFPERR_OFPTFFC_BAD_LEN;
> + }
> +
> + /* update props' header len */
> + table_features_move_data((uint8_t*)&otf, p, length, sizeof(otf));
> +
> + /* length -> n array */
> + tf->length = ntohs(otf.length); /* now we get length of this tf */
> + 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_len = tf->length - sizeof(otf);
> + if ((error = decode_table_feature_props(p, props_len, tf)))
> + goto out;
> +
> + /* if succeed, update length after decode props */
> + if (*length > props_len)
> + *length -= props_len;
> + else {
> + VLOG_DBG("Table features decode bad length, "
> + "length left(%u), properties length(%u).\n", *length, props_len);
> + return OFPERR_OFPTFFC_BAD_LEN;
> + }
> +
> +out:
> + return error;
> +}
> +
> +static enum ofperr
> +ofputil_pull_table_features_raw(uint8_t *p, uint32_t length,
> + struct ofputil_table_features tfs[],
> + int *tfs_num)
> +{
> + enum ofperr error = 0;
> + int i = 0;
> +
> + /* FIXME the 0xff is hard coding */
Please fix it.
> + while (length > 0 && i <= 0xff) {
> + struct ofputil_table_features *tf = &tfs[i];
> +
> + if ((error = decode_table_feature_raw(&p, &length, tf))) {
> + goto out;
> + }
> + ++i;
> + }
> +out:
> + *tfs_num = i;
> + return error;
> +}
> +
> +enum ofperr
> +ofputil_decode_table_features_reply(const struct ofp_header *oh, int *tfs_num,
> + struct ofputil_table_features tfs[])
enum ofperr
ofputil_decode_table_features_reply(const struct ofp_header *oh, int *tfs_num,
struct ofputil_table_features tfs[])
> +{
> + struct ofpbuf msg;
> + enum ofpraw raw;
> + uint8_t *p;
> + int features_len = 0;
> + int error = 0;
> +
> + ofpbuf_use_const(&msg, oh, ntohs(oh->length));
> + raw = ofpraw_pull_assert(&msg);
> + if (raw != OFPRAW_OFPST13_TABLE_FEATURES_REPLY) {
> + VLOG_DBG("bad msg type(%u)\n", raw);
Should this logging be rate limited?
> + return OFPERR_OFPBRC_BAD_TYPE;
> + }
> +
> + /* 8 is multipart-header's len */
> + features_len = ntohs(oh->length) - sizeof(*oh) - 8;
What is 8?
Can it be a #define or similar?
I see this calculation is made more than once.
Could it factored out into a function?
> + p = ofpbuf_try_pull(&msg, features_len);
> + if (!p) {
> + VLOG_WARN("table features length %u is longer than space "
> + "in message length %zu", features_len, msg.size);
Should this logging be rate limited?
> + return OFPERR_OFPTFFC_BAD_LEN;
> + }
> +
> + ofputil_pull_table_features_raw(p, (uint16_t)features_len, tfs, tfs_num);
> +
> + return error;
> +}
> +
> +
> +enum ofperr
> +ofputil_decode_table_features_request(const struct ofp_header *oh, int *tfs_num,
> + struct ofputil_table_features tfs[],
> + uint32_t *flag)
enum ofperr
ofputil_decode_table_features_request(const struct ofp_header *oh, int *tfs_num,
struct ofputil_table_features tfs[],
uint32_t *flag)
> +{
> + struct ofpbuf msg;
> + enum ofpraw raw;
> + uint8_t *p;
> + uint32_t features_len;
> + 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) {
> + VLOG_DBG("bad msg type(%u)\n", raw);
Should this logging be rate limited?
> + return OFPERR_OFPBRC_BAD_TYPE;
> + }
> +
> + if (msg.size == 0) /* stands for GET table_features request */ {
> + *flag = OTF_GET;
> + } else {
> + *flag = OTF_SET;
> +
> + /* 8 is multipart-header's len */
> + features_len = ntohs(oh->length) - sizeof *oh - 8;
> + p = ofpbuf_try_pull(&msg, features_len);
> + if (!p) {
> + VLOG_WARN("table features length %u is longer than space "
> + "in message length %zu", features_len, msg.size);
Should this logging be rate limited?
> + return OFPERR_OFPTFFC_BAD_LEN;
> + }
> +
> + ofputil_pull_table_features_raw(p, features_len, tfs, tfs_num);
> + }
> + 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;
> + int error = 0;
> + uint8_t *data;
> + int data_len;
> + int padding_len;
> +
> + for (i = 0; i < prop_num; i++) {
> + struct ofp13_table_feature_prop_header *oprop;
> + const struct ofputil_table_feature_prop_header *prop = &props[i];
> + if (!prop->data || !prop->length) {
> + continue;
> + }
> +
> + oprop = ofpbuf_put_zeros(reply, sizeof *oprop);
> + data_len = prop->length - sizeof *oprop;
> + padding_len = ROUND_UP(prop->length, 8) - prop->length;
> +
> + oprop->type = htons(prop->type);
> + oprop->length = htons(prop->length);
> +
> + data = ofpbuf_put_uninit(reply, data_len + padding_len);
> + memcpy(data, prop->data, data_len);
> + memset(data + data_len, 0, padding_len);
> +
> + if (error != prop_data_hton(prop->type, prop->length, data))
> + return error;
> + }
> +
> + return 0;
> +}
> +
> +/* use it when encode */
> +
> +static uint32_t
> +table_feature_prop_calculate_len(
> + const struct ofputil_table_feature_prop_header *prop)
> +{
> + /* NOTE, ofputil prop should padding now, FIXME later */
Does it work as is? If not, please fix it.
If so, is the comment still relevant?
> + return ROUND_UP(prop->length, 8);
Could 8 be a #define or similar?
> +}
> +
> +static uint32_t
> +table_feature_props_calculate_len(
> + const struct ofputil_table_feature_prop_header *props,
> + uint16_t n_property)
static uint32_t
table_feature_props_calculate_len(const struct ofputil_table_feature_prop_header *props,
uint16_t n_property)
> +{
> + int i;
> + uint32_t len = 0;
> +
> + for (i = 0; i < n_property; i++) {
> + len += table_feature_prop_calculate_len(&props[i]);
> + }
> +
> + return len;
> +}
> +
> +static uint32_t
> +table_feature_calculate_len(const struct ofputil_table_features *tf)
> +{
> + /* 64 is the header length */
I don't see 64 used in this function.
> + uint32_t len = sizeof(struct ofp13_table_features);
> +
> + len += table_feature_props_calculate_len(tf->props, tf->n_property);
> +
> + return len;
> +}
> +
> +static void
> +ofputil_put_table_feature(const struct ofputil_table_features *tf, struct ofpbuf *reply)
static void
ofputil_put_table_feature(const struct ofputil_table_features *tf,
struct ofpbuf *reply)
> +{
> + struct ofp13_table_features *otf;
> + uint32_t feature_len = table_feature_calculate_len(tf);
> + otf = ofpbuf_put_zeros(reply, 64); //feature_len
If feature len is 64 then can you use it instead of 64 in the line above?
> +
> + /* if it's a get request, length is 64 bits. */
s/64/feature_len/ ?
> + otf->length = htons(feature_len);
> + 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);
> +
> + /* encode props */
> + if (tf->n_property > 0) {
> + ofputil_encode_table_features_props(reply, tf->props, tf->n_property);
> + }
I don't think you need to guard the call to
ofputil_encode_table_features_props() as it will do nothing if
tf->n_property is zero.
> +}
> +
> +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) {
I'm not sure that I understand why the check against
OFPRAW_OFPST13_TABLE_FEATURES_REPLY is necessary.
Is it possible to get here in other cases?
If so is it valid to more or less do nothing?
Also, in that case is the call to ofpmp_postappend() valid?
> + 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);
> +
> + }
> + 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);
request = ofpraw_alloc(OFPRAW_OFPST13_TABLE_FEATURES_REQUEST,
ofp_version, 0);
> + break;
> + }
The { } are not needed for the OFP12_VERSION / OFP13_VERSION case.
> + default:
> + NOT_REACHED();
> + }
> +
> + return request;
> +}
> +
> +struct ofpbuf *
> +ofputil_encode_table_features(const struct ofputil_table_features tfs[], int n,
> + const struct ofp_header *request)
struct ofpbuf *
ofputil_encode_table_features(const struct ofputil_table_features tfs[], int n,
const struct ofp_header *request)
> +{
> + struct ofpbuf *reply;
> + int i;
> +
> + /* should we replace the func to alloc_features? */
I'm not sure that I understand the comment above.
Is it still relevant?
> + reply = ofpraw_alloc_stats_reply(request, 0);
> +
> + for (i = 0; i < n; i++) {
> + /* TODO encode body inside the func */
> + ofputil_put_table_feature(&tfs[i], reply);
> + }
> +
> + return reply;
> +}
> +
> /* ofputil_table_mod */
>
> /* Decodes the OpenFlow "table mod" message in '*oh' into an abstract form in
> --
> 1.7.3.1.msysgit.0
>
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
More information about the dev
mailing list