On Thu, Jun 02, 2016 at 11:52:31PM +0800, Benli Ye wrote:
> It is meaningful for user to check the stats of IPFIX.
> Using IPFIX stats, user can know how much flows the system
> can support. It is also can be used for performance check
> of IPFIX.
Thanks for the patch!
Clang reports:
../lib/ofp-print.c:3315:49: error: format specifies type 'size_t' (aka
'unsigned int') but the argument has type 'uint64_t' (aka 'unsigned long long')
[-Werror,-Wformat]
sparse reports:
../ofproto/ofproto-dpif-ipfix.c:1409:31: warning: constant
0xffffffffffffffff is so big it is unsigned long long
I suggest changing the definition of COLLECTOR_SET_ID_UNUSED to
UINT64_MAX.
In nx_ipfix_stats_reply, why is 'collector_set_id' a 64-bit value if its
range is only 32-bit:
+ ovs_be64 collector_set_id; /*range 0 to 4,294,967,295*/
In ofp-msgs.h, please put a space at the beginning of these comments:
+ /*NXST 1.0+ (3): struct nx_ipfix_stats_reply. */
...
+ /*NXST 1.0+ (4): struct nx_ipfix_stats_reply[]. */
Please add a test to tests/ofp-print.at for each new message.
In ofp_print_nxst_ipfix_flow_reply(), the inner parentheses below are
unneeded. Please remove them:
+ if ((is.collector_set_id) < 10) {
...
+ if ((is.collector_set_id) < 100) {
However, it looks like you're just trying to get some kind of alignment
in the next printf statement, so why not just write "%3"PRIuSIZE
instead and drop the if statements?
Regarding the name of ofputil_decode_ipfix_stats(), for cases where
there can be more than one set of stats usually we use "pull" instead of
"decode" in the function name.
In ofputil_decode_ipfix_stats(), the "bad_len" check should never fail,
because ofpraw_decode() or ofpraw_pull() should check that the message
length is correct.
In ofputil_count_ipfix_stats(), it shouldn't be necessary to decode all
the stats to count them: since they are fixed-length, division should be
enough.
For collectors_send(), uint16_t seems like a strange return type. I'd
use size_t since that the type of the 'n' parameter. Similarly for
other functions that return the number of failures.
Do you think it's valuable to have both struct ofproto_ipfix_stats and
struct ofputil_ipfix_stats? They are almost the same structure.
In ipfix_get_stats__(), use struct assignment instead of memcpy,
e.g. change
memcpy(stats, &exporter->stats, sizeof(*stats));
to
*stats = exporter->stats;
Do we need both COLLECTOR_SET_ID_NONE and COLLECTOR_SET_ID_UNUSED?
Please don't put parentheses around the return expression in
get_ipfix_stats():
+ return (dpif_ipfix_get_stats(di, bridge_ipfix, replies));
in conformance with CodingStyle.md:
Do not put gratuitous parentheses around the expression in a return
statement, that is, write "return 0;" and not "return(0);"
Please add some more details to the comment on get_ipfix_stats in
ofproto-provider.h. Please also don't put 'const' on the non-pointer
parameter there:
+ /*
+ * Get IPFIX stats
+ **/
+ int (*get_ipfix_stats)(
+ const struct ofproto *ofproto,
+ const bool bridge_ipfix,
+ struct ovs_list *replies
+ );
In tests/ofproto-dpif.at, please add a test for flow IPFIX statistics.
Please also add "negative" tests, that is, tests that simply return an
error message because no bridge or flow IPFIX is enabled.
Please document the new ovs-ofctl commands in the ovs-ofctl manpage.
Please mention that these commands use an Open vSwitch extension that is
only in Open vSwitch 2.6 and later.
Please add an item to NEWS mentioning the new feature.
Thanks,
Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
