Thanks for your reply!
On 01/11/2013 12:29, Simon Horman wrote:
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...@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)
I'll fix it, thanks.
+{
+ int i = 0;
+ int n = 0;
+ int element_size = (int)get_prop_length(prop->type);
Blank line here please.
I'll fix it, thanks.
+ 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?
Currently if the type is not in OFPTFPT13_*, get_prop_length
would return 0. So I think it should return OFPERR_OFPTFFC_BAD_TYPE.
Thanks!
+ 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?
Yes, I'm trying to fix it. But there are 2 ways to fix it.
A. Print act/inst/oxm bitmap in console.
B. Print human-readable texts in console.
Which one do you think is better?
A -> human-unreadable
B -> lots of texts in console
+ 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.
Same.
+ 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.
Agreed. I'll fix it to ofpbuf_pull, thanks.
+{
+ 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?
Agreed. I'll fix it, thanks.
+ 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)
Agreed, I'll fix it, thanks.
+{
+ int n = 0;
+ if (length % elem_size)
+ return OFPERR_OFPTFFC_BAD_LEN;
if (length % elem_size) {
return OFPERR_OFPTFFC_BAD_LEN;
}
I'll fix it, thanks.
+
+ 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.
Agreed. The code is quick and dirty, I'll fix it to ofputil_*.
There is a trick here:
I init table features by ofp13_*, and when it needs encoding, I
translate it by the function above directly.
It's not a good trick. I'm fixing it, thanks.
+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.
Agreed. I'll fix it to ofputil_*, thanks.
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.
Agreed. I'll fix it to ofputil_*, thanks.
In this case I think you should supply an array of ovs_be32 to
save the values into.
Not understood. Did you mean this? :
static void ntoh_be32_array(ovs_be32 *array, uint16_t n_elem)
+
+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.
Agreed. I'll fix it to ofputil_*, thanks.
+
+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.
Agreed. I'll fix it to ofputil_*, thanks.
+
+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.
Agreed. I'll fix it to ofputil_*, thanks.
+
+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.
Agreed. I'll fix it, thanks.
+ 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.
Agreed. I'll fix them to switch-case, thanks!
+ 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.
Agreed. I'll fix them, thanks.
+ {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?
I'm planing to replace the big structure to switch-case.
I'm doing it now.
+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?
Agreed. I'll fix it to:
typedef void (*prop_array_trans_func)(uint8_t*, uint16_t);
Thanks!
+{
+ 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
Ditto, thanks.
+{
+ 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.
Agreed. Thanks for your suggestions. I'm fixing it to ofputil_*.
+{
+ 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)
Agreed. I'll fix it. Thanks!
+{
+ 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)
Agreed. I'll fix it. Thanks!
+{
+ 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)
Agreed. I'll fix it. Thanks!
+{
+ 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 the prop is (ofputil_table_feature_prop_header *),
so I calculate it with (struct ofp13_table_feature_prop_header).
+
+ /* 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?
Agreed.
8 is property-align.
I'll fix it with a marco, thanks.
+ 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.
Agreed. I'll fix it to return directly, thanks.
+ } 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?
Agreed, I'll fix it with &bad_ofmsg_rl, thanks.
+ 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.
Got it, thanks.
+ 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[])
I'll fix it, thanks.
Is it in CodingStyle?
+{
+ 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?
I'll fix it to "VLOG_DBG_RL(&bad_ofmsg_rl,", thanks!
+ 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?
Yes, I'll write a func to calculate the table features data_len.
And #define MULTIPART_HDR_LEN 8
BTW, I see other code using 8 directly, perhaps they should be
normalized too?
--
case OFPMT_OXM:
if (padded_match_len) {
*padded_match_len = ROUND_UP(match_len, 8);
}
--
if (len < sizeof *oheh || !ofpbuf_try_pull(&msg, ROUND_UP(len, 8))) {
return false;
}
+ 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)
This one would be better, I think.
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?
Yes. I'll fix it, thanks.
+ 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?
Yes. I'll fix it, thanks.
+ 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?
Yes... It's lack of comment.
I made the prop alloc with padding, but the length is as ofp13_*
Like this:
12345678
TTLLPPPP
T = Type
L = Length
P = Padding
The length here is 4, but the padding exists, so I ROUND_UP it to 8.
It's somewhat dubious, I'll fix it with a normalized translation.
(Now I'm using a quick translation as ofp13_* to ofp13_* you see above)
+}
+
+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)
Could the length > 80?
+{
+ 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.
Yes, the comment is useless. I'll delete it.
+ 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)
Thanks, I'll fix it.
+{
+ 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?
64 is sizeof(*otf) actually.
The comment is just to remind me if I could use feature_len instead.
I'll fix it to sizeof(*otf), thanks
feature_len is not 64, it's the actual feature's len.
+
+ /* if it's a get request, length is 64 bits. */
s/64/feature_len/ ?
It's 64 bytes. I'll correct it.
This comment means:
if OFPMP_TABLE_FEATURES request contains no body:
it's a GET request.
else
it's a SET request.
+ 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.
Yes. You're right, this code is defensive.
I'll remove it soon.
+}
+
+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?
I use the code to defend it from other caller.
I'll add else:
} else {
NOT_REACHED();
}
+ 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);
Agreed, I'll fix it, thanks.
+ 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?
Openflow defines some objects to get:
desc, stats, features, config, object itself.
So I think the function could be replaced with a meaningful name.
+ 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@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
.
--
Best Regards
Alexander Wu
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
