[ovs-dev] [PATCH] OF1.5/EXT-334 OXS/Extensible Flow Entry Statistics Support

[Ben Pfaff]

On Wed, Sep 13, 2017 at 05:12:11PM +0530, SatyaValli wrote:
> From: SatyaValli <satyavalli.rama at 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.