On Wed, Sep 13, 2017 at 05:12:11PM +0530, SatyaValli wrote: > From: SatyaValli <satyavalli.r...@tcs.com> > > This Patch provides implementation Existing flow entry statistics are > redefined as standard OXS(OpenFlow Extensible Statistics) fields for > displaying the arbitrary flow stats.The existing Flow Stats were renamed > as Flow Description.
Thank you for doing all this work. I have some comments. I think that the comment on 'reserved' in struct ofp_oxs_stat is wrong. This field should always be zero, not "One of OFPST_*.". Why does "struct ox_fields" have a plural name? It appear to represent a single field. The meaning of the new 'oxs_fields' and 'flow_desc' members in ofputil_flow_stats_request isn't clear from the comments: - It looks like oxs_fields is used to append some stats to flow stats request messages, but I can't find a description of this ability in the OF1.5.1 spec. I see a mention of stats in *replies* to these messages, but not requests. Can you help me understand? - I guess that 'flow_desc' distinguishes OFPMP_FLOW_DESC from OFPMP_FLOW_STATS. That really isn't clear from the comments. Please edit the top-level comment for this struct to explain the three kinds of messages it can represent and how they are distinguished. Names like "oxs-idle_time" are going to be hard to remember. Do you think that the "oxs-" prefix is really needed? Do you think that we could consistently use either _ or -, not a combination of both? I don't understand this comment. The grammar is a little funny: + /* ofputil_flow_stats structure is used to encode/decode oxs stats + * fields since no stat fields are necessary in request NULL was + * passed */ There is a little funny indentation in ofputil_decode_flow_stats_reply(). Please check it. ox-stat.c seems to use a function name be64toh() on uint64_ts that actually contain big-endian data. Please instead use ovs_be64 and ntohll(). Possibly a helper function or two would help. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev