On Sun, May 03, 2015 at 05:35:43PM -0700, shashank.shanb...@gmail.com wrote:
> From: Shashank Shanbhag <shashank.shanb...@gmail.com>
>
> Fix replace-flows and diff-flows to modify/diff flows in multiple tables.
> Add a --tables option that allows the user to specify a comma-separated
> list of table indexes to replace/diff.
>
> Signed-off-by: Shashank Shanbhag <shashank.shanb...@gmail.com>
> Acked-by: Romain Lenglet <romain.leng...@oracle.com>
Thanks for the patch! This fixes a long-standing bug. Sorry about
the delayed review--I'm doing a huge amount of traveling this month
and everything is backlogged.
"git am" pointed out some whitespace errors:
Applying: ovs-ofctl: replace-flows and diff-flows support for multiple
tables
/home/blp/ovs/.git/rebase-apply/patch:708: trailing whitespace.
/home/blp/ovs/.git/rebase-apply/patch:730: trailing whitespace.
/* Setup connection to switch and generate dump request. */
/home/blp/ovs/.git/rebase-apply/patch:761: trailing whitespace.
struct fte_version *b = fte->versions[1];
warning: 3 lines add whitespace errors.
The build process pointed out another minor issue:
utilities/ovs-ofctl.c
Files listed above unexpectedly #include <assert.h>.
Please use ovs_assert (from util.h) instead of assert.
I have some more specific comments too.
utilities/ovs-ofctl.8.in
------------------------
Here, I would put the option before the command name, to match the
existing style:
-.IP "[\fB\-\-readd\fR] \fBreplace\-flows \fIswitch file\fR"
+.IP "[\fB\-\-readd\fR] \fBreplace\-flows \fIswitch file\fR [\fB\-\-tables
\fltable-ids\fR]"
Reads flow entries from \fIfile\fR (or \fBstdin\fR if \fIfile\fR is
\fB\-\fR) and queries the flow table from \fIswitch\fR. Then it fixes
up any differences, adding flows from \fIflow\fR that are missing on
\fIswitch\fR, deleting flows from \fIswitch\fR that are not in
\fIfile\fR, and updating flows in \fIswitch\fR whose actions, cookie,
or timeouts differ in \fIfile\fR.
Similarly for diff-flows.
utilities/ovs-ofctl.c
---------------------
It's unusual to dynamically allocate an ovs_list head. I would
ordinarily just declare 'tables' as a plain ovs_list instead of a
pointer.
Do you think there is much value in maintaining the order of table
update using the linked list? It seems likely to me that ordinarily
the update order is not important, and in cases where it is important
it seems that the user could call ovs-ofctl more than once. It would
simplify the implementation a little, and I guess that it would be
just as easy to explain, if the tables were simply updated in order of
increasing table number.
In get_table_classifier(), there should be a space before the (,
because this macro is supposed to act like a "for" or other loop and
our style is to put a space there (e.g. "for (...)"), and the { should
be on the same line:
+ HMAP_FOR_EACH_IN_BUCKET(ht_cls_node, hnode, table_id, ht_cls)
+ {
The return type should be on a line of its own here:
+int table_ids_list_and_bitmap_from_string(struct ovs_list **tables,
+ unsigned long **bmtables,
+ const char *s)
In read_flows_from_file() and read_flows_from_switch(), the code
previously called classifier_defer() before the main loop and
classifier_publish() afterward, but now it calls it just before and
after each insertion. I think that's a bad idea because it could
cause pathological performance in some cases. It would be better to
do it the old way; one could, for example, call classifier_defer() as
part of inserting a new classifier.
I think that we might as well just use an array for the table of
classifiers. It's only an array of 255 elements, after all. I don't
see a benefit to using a hash table.
In read_flows_from_switch(), it seems odd to me to use
parse_ofp_flow_stats_request_str() to initialize the flow stats
request. I would tend to continue initializing it "by hand".
For the case where exactly one table is diffed, it might be worthwhile
to have read_flows_from_switch() just retrieve that table, by setting
table_id in the flow_stats_request.
delete_extra_switch_flows(), add_update_switch_flows(), and
ofctl_diff_flows() all contain a lot of duplicated code. Please try
to reduce it (with helper functions?).
Thanks again!
Ben
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
