Hi Ben,
Thanks for your comments, and for making time to review the patch.
On Thu, May 7, 2015 at 12:31 PM, Ben Pfaff <b...@nicira.com> wrote:
> 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'll correct those.
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.
>
I think update order matters. I would expect table 0 to be updated last
after all the flows in later tables are updated to, say, reflect a change
in policy.
Also, calling ovs-ofctl 255 times in the worst case would be really
expensive, considering the fact that the FILE can be large, and the
connection may be over SSL.
>
> 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.
>
Trying to avoid the array of 255 elements, especially for cases where there
are less number of tables being replaced.
> 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".
>
I'd like to avoid doing it this way. Any future updates will need to be
done at a single place. Also, parse_ofp_flow_stats_request_str() is not
being used anywhere in the code, FYI.
> 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?).
>
Yes. I'll take care of these as well.
>
> Thanks again!
>
> Ben
>
Please let me know what you think,
Thanks,
Shashank
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
