[ovs-dev] [PATCH 2/4] Implement the encode/decode Table Features functions
Simon Horman
horms at verge.net.au
Tue Nov 5 01:14:16 UTC 2013
On Fri, Nov 01, 2013 at 07:52:35PM +0800, Alexander Wu wrote:
> 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.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)
> >
>
> 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
I think that in general B is preferable.
But that A would be better than nothing.
There is precedence for printing bitmaps to user-space for
other messages that have bitmap fields.
>
> >>+ 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).
Thanks, sorry for missing that.
Is this calculation used elsewhere?
If so it might be useful to make a helper-function.
>
> >>+
> >>+ /* 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?
I think so.
> >>+{
> >>+ 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;
> }
I think that is a good idea.
In a separate patch.
>
> >>+ 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.
Thanks, that sounds reasonable.
> >>+ 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
> >>
> >
> >.
> >
>
>
> --
> Best Regards
> Alexander Wu
>
More information about the dev
mailing list