Hi Ben,
Thank you for reviewing the patch.
>"sparse" reported these warnings:
All the sparse warnings have been resolved
>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.
This comment has been incorporated in the new patch.
>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.
The code has been changed for the same.
>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().
Table description code has been changed in accordance with the mentioned
comments.
>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 has been incorporated.
>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.
Related code has been grouped together in ofp-util.c
>Please check the indentation in ofputil_decode_table_desc(). At least
>the "while" statement seems to be mis-indented.
Done.
>ofputil_decode_table_mod() still doesn't understand any properties, so
>I would not remove the comment that mentions that.
Done.
>Please don't comment out code, as done in ofputil_encode_table_mod().
Done.
>I don't see a benefit of having a nested structure inside struct
>ofputil_table_desc.
Code has been changed for the same
>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. */
Done the suggested changes.
>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;
> }
We have rechecked the code, the higher-level code is not performing these
checks.
Also same checking has been done for handle_table_feature_request()
>One of the lines in rule_eviction_priority() is too long. Please read
>CodingStyle.md. Ditto for eviction_group_add_rule().
Done the suggested formatting.
>Please document your new features in ovs-ofctl.8.in and mention it in
>NEWS.
Added the new feature in NEWS, ovs-ofctl.8.in and DESIGN.md files.
We will send the updated patch.
Thanks and Regards,
Saloni
=====-----=====-----=====
Notice: The information contained in this e-mail
message and/or attachments to it may contain
confidential or privileged information. If you are
not the intended recipient, any dissemination, use,
review, distribution, printing or copying of the
information contained in this e-mail message
and/or attachments to it are strictly prohibited. If
you have received this communication in error,
please notify us by reply e-mail or telephone and
immediately and permanently delete the message
and any attachments. Thank you
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
