Thanks for the revision. I have some more comments. On Mon, Jun 19, 2017 at 01:14:40PM +0530, SatyaValli wrote: > commit 49a3592b2878a33033e5fd2e6e5ce82ebccef780 > Author: Satya Valli <satyavalli.r...@tcs.com> > Co-authored-by: Lavanya Harivelam <harivelam.lava...@tcs.com> > Co-authored-by: Surya Muttamsetty <muttamsetty.su...@tcs.com>
None of the above should be at the top of the commit message. Coauthors go with the sign-offs at the end. > OpenVswitch: OF1.5/EXT-334 Extensible Flow Entry Statistics Please don't repeat the subject again in the body. > OpenFlow 1.5 introduces the Extensible Statistics (OXS) by redefining the > existing > flow entry statistics with OXS Fields. > This Patch provides implementation for OXS fields encoding in TLV format. > > To support this implementation below two messages are newly added > > OFPST_OXS_FLOW_STATS_REQUEST > OFPST_OXS_FLOW_STATS_REPLY > OFPST_OXS_AGGREGATE_STATS_REQUEST > OFPST_OXS_AGGREGATE_STATS_REPLY > OFPST_FLOW_REMOVED > As per the openflow specification-1.5, this enhancement should take place > on the existing flow entry statistics with the OXS fields on all the messages > that carries flow entry statistics. > > The current commit adds support for the new feature in flow statistics > multipart messages, > aggregate multipart messages and OXS flow statistics support for flow removal > message. > > Some more fields are added to ovs-ofctl dump-flows command to support > OpenFlow15 OXS stats. > Below are Commands to display OXS stats field wise > > Flow Statistics Multipart > ovs-ofctl dump-flows -O OpenFlow15 <bridge> oxs-duration > ovs-ofctl dump-flows -O OpenFlow15 <bridge> oxs-idle_time > ovs-ofctl dump-flows -O OpenFlow15 <bridge> oxs-packet_count > ovs-ofctl dump-flows -O OpenFlow15 <bridge> oxs-byte_count > > Aggregate Flow Statistics Multipart > ovs-ofctl dump-aggregate -O OpenFlow15 <bridge> oxs-packet_count > ovs-ofctl dump-aggregate -O OpenFlow15 <bridge> oxs-byte_count > ovs-ofctl dump-aggregate -O OpenFlow15 <bridge> oxs-flow_count > > Make Check Changes in - ofproto.at and ofproto-dpif.at: > For 1.5 version "flags" variable has been removed from flow_stats_reply > structure. > For avoiding the make check failures for OF1.5 version add-flow with flags, > below test cases has been modified for 1.5 version in respective .at files > > Files testcase > ofproto.at 0884 > ofproto-dpif 1126 Please don't refer to tests by number. The numbers are not meaningful and change often. Use the name of the test. > In ofproto.at there is one test case numbered 0884 and 1126 in ofproto-dpif.at > which is validating flagsfor OF1.5 this testcase is not valid, because > in stats reply flags are not explicitly communicated,I've removed this test > case. > Before removing this test case is failing for 1.5 version. > > Since FLOW_REMOVED is not supported in OF1.5 version, > testing has been done by adding test case in ofproto.at. > While doing make check after adding test case we are able to see > OFPT_FLOW_REMOVED (OF1.5) message has sent successfully. > > But make check is failing because of Signal 15 termination, > so we've removed that test case from the patch. Why isn't FLOW_REMOVED supported in OF1.5? Support is needed. Signal 15 is SIGTERM, meaning that something actually used "kill" to kill it intentionally. Please investigate and fix the problem rather than removing a test. This adds features that users can use via ovs-ofctl, but it does not document them. Please document them in ovs-ofctl(8). Please also add appropriate NEWS items to explain the new features. This adds support for new OpenFlow messages, but it does not add any tests for printing them in ofp-print.at. Please add at least one representative test there for every new message. struct ofp15_flow_removed doesn't seem to be the same as ofp_flow_removed in OpenFlow 1.5. This adds a global variable oxs_field_set. This is inappropriate. Do not use global variables. The variable oxs_field_set has a bunch of unnamed magic numbers. This is inappropriate. Do not use unnamed magic numbers. This code is weird. Fix it please: + if (parse_oxs_field(name, &f)) { + if (f->fl_type == OFPXST_OFB_DURATION) { + oxs_field_set |= (1 << f->fl_type); + } else if (f->fl_type == OFPXST_OFB_IDLE_TIME) { + oxs_field_set |= (1 << f->fl_type); + } else if (f->fl_type == OFPXST_OFB_FLOW_COUNT) { + oxs_field_set |= (1 << f->fl_type); + } else if (f->fl_type == OFPXST_OFB_PACKET_COUNT) { + oxs_field_set |= (1 << f->fl_type); + } else if (f->fl_type == OFPXST_OFB_BYTE_COUNT) { + oxs_field_set |= (1 << f->fl_type); + } This patch seems not to recognize that OF1.5+ has two different ways to request and get replies for flow stats: FLOW_DESC and FLOW_STATS. It looks like it only supports the latter. We must support both. Thanks, Ben. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev