On Fri, Nov 07, 2014 at 12:31:16PM +0530, Saloni Jain wrote:
> Also attaching the patch for your reference.
Thank you for the patch. I have some comments.
"sparse" reported these warnings:
../lib/ofp-util.c:77:17: warning: non-ANSI function declaration of function
'get_ovsprop_map'
../lib/ofp-util.c:5012:12: warning: incorrect type in assignment (different
base types)
../lib/ofp-util.c:5012:12: expected int enum ofputil_table_config [signed]
config
../lib/ofp-util.c:5012:12: got restricted ovs_be32
../lib/ofp-util.c:5013:17: warning: incorrect type in assignment (different
base types)
../lib/ofp-util.c:5013:17: expected restricted ovs_be32 [usertype] config
../lib/ofp-util.c:5013:17: got int enum ofputil_table_config [signed] config
../lib/ofp-util.c:5273:16: warning: incorrect type in assignment (different
base types)
../lib/ofp-util.c:5273:16: expected int enum ofputil_table_config [signed]
config
../lib/ofp-util.c:5273:16: got restricted ovs_be32
../lib/ofp-util.c:5278:21: warning: incorrect type in assignment (different
base types)
../lib/ofp-util.c:5278:21: expected restricted ovs_be32 [usertype] config
../lib/ofp-util.c:5278:21: got int enum ofputil_table_config [signed] config
The config field in a table description is a collection of bit-fields,
but some of this code, such as ofp_print_table_config() and
ofputil_append_table_desc_reply(), treats it as if it were a single
enumeration. Please change the code to regard it bitwise.
Here, in ofp-util.c, please write (void) instead of ():
static const struct ovstableprop_map *
get_ovsprop_map()
{
However, I don't see any value in the mapping layer between
OFPTMPEF14_* and OFPUTIL_TABLE_EVICTION_*. I would drop it and just
use the OFPTMPEF14_* values directly.
I don't understand the parse_table_property() function. It doesn't
seem to parse anything like the description of struct
ofp_table_mod_prop_eviction from OpenFlow 1.4. Ditto for
put_table_mod_eviction_property().
In ofputil_encode_table_desc_request(), I would use an "if" statement
to report an error for ofp_version < OFP14_VERSION. That should cause
less trouble as we add support for newer OpenFlow versions.
This commit adds a lot of related code to ofp-util.c, but it spreads
it around the file. I would expect that it would all be grouped
together since it is related.
Please check the indentation in ofputil_decode_table_desc(). At least
the "while" statement seems to be mis-indented.
ofputil_decode_table_mod() still doesn't understand any properties, so
I would not remove the comment that mentions that.
Please don't comment out code, as done in ofputil_encode_table_mod().
I don't see a benefit of having a nested structure inside struct
ofputil_table_desc.
There is a missing space before the { here. Also it doesn't make sense to
mention 0xff here for table_id since it's not a valid value (see the decoding
function):
/* Abstract ofp14_table_desc. */
struct ofputil_table_desc{
uint8_t table_id; /* ID of the table, 0xff indicates all
tables. */
There is no need for this code in handle_table_desc_request() because
higher-level code has already checked these properties for
correctness:
ofpbuf_use_const(&msg, request, ntohs(request->length));
ofpraw_pull_assert(&msg);
if (ofpbuf_size(&msg) || ofpmp_more(request)) {
return OFPERR_OFPTFFC_EPERM;
}
One of the lines in rule_eviction_priority() is too long. Please read
CodingStyle.md. Ditto for eviction_group_add_rule().
Please document your new features in ovs-ofctl.8.in and mention it in
NEWS.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
