On Mon, Jun 25, 2012 at 11:13:34AM -0700, Arun Sharma wrote:
> ovs-ofctl will have --sort[=field]|--rsort[=field] option for 'dump-flows'.
> Sort by priority if no field argument is specified or sort by field.
> "--sort" to go in ascending order and "--rsort" to go in descending
> order for flow fields other then priority field. In case of priority field,
> output should be reversed with "--sort" going in descending order and
> "--rsort" going in ascending order.
>
> Feature# 8754
> Signed-off-by: Arun Sharma <[email protected]>
GCC says:
utilities/ovs-ofctl.c:478:39: error: bad constant expression
lib/ofp-print.c: In function ‘ofp_print_version’:
lib/ofp-print.c:1605: error: unused parameter ‘type’
I don't understand why the priority field sort order is the opposite
of other fields. I'd drop that.
The ofp_flow_stats_reply() function needs a verb in its name, and the
'reply' part seems unnecessary. Perhaps ofp_format_flow_stats().
It's odd that it inserts a new-line before its output.
In a function definition, put the outermost { on a line by itself.
In a function definition, put the return type on a line by itself.
In the tests, it'd be better to use ofctl_strip, as used in other
tests, rather than the custom "tr" and "awk" invocations.
In the documentation, the second option below includes the syntax for
the first, so you can omit the first.
.IP "\fB\-\-sort\fR"
.IQ \fB\-\-sort\fR[\fB=\fIfield\fR]
In the documentation, please mention how the user may determine the
supported field names.
In ovs-ofctl.c, there is no need to make a copy of optarg; you may use
it by pointer directly.
In parse_options(), you didn't document -s or -r so please add OPT_*
constants instead of using these short forms.
Please don't use subtraction for obtaining a strcmp()-like return
value because of the pitfalls of overflow (even though overflow may
not be a problem here). Instead, use an expression like:
return a < b ? -1 : a > b;
or similar.
Please don't insert special cases in the comparison functions for
ascending versus descending. Instead, just compute the correct
comparison for one sort order, then at the very end invert the result
if the opposite sort order is in use.
I will probably have further comments after you finish up with these.
Thanks,
Ben.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
