On Mon, May 07, 2018 at 10:11:44PM +0530, SatyaValli wrote:
> From: SatyaValli <[email protected]>
>
> This Patch provides implementation Existing flow entry statistics are
> redefined as standard OXS(OpenFlow Extensible Statistics) fields for
> displaying the arbitrary flow stats.
Thanks for the revision.
I see that you used "LW" to stand for "lightweight", but I don't see
anything that explains that.
I don't see anything that actually implements the lightweight flow
requests and replies; for example, ofp-flow.[ch] doesn't seem to have
any way to decode or encode them. (If you don't intend to implement
them here, then it may not be useful to define OFPTYPE_* and OFPRAW_*
for them.)
I don't see a reason to separate OFPRAW_OFPST11_FLOW_REQUEST and
OFPRAW_OFPST15_FLOW_REQUEST. They have the same format and the same
code is used to process both.
In lib/automake.mk, please indent with a tab to match the rest of the
file.
ofputil_decode_flow_removed() must honor the error return value from
oxs_flow_removed_stat_pull().
This does not use any OVS acceptable style:
if (ofputil_pull_ofp11_match
(msg, NULL, NULL, &fs->match, &padded_match_len)) {
The following is one acceptable variation:
if (ofputil_pull_ofp11_match(msg, NULL, NULL, &fs->match,
&padded_match_len)) {
The use of oxs_field_set in ofputil_append_flow_stats_reply() puzzles
me. It is initialized using a loop, which is odd given that it's really
a static value with all of the relevant bits set to 1, and then that's
passed to oxs_put_stat() (this function is the only caller), and then
that passes it to ox_put_raw() (again, this function is the only
caller), and then ox_put_raw() checks that each bit is set. Is there
some big plan here? Why can't ox_put_raw() just append all the fields
without needing this oxs_field_set parameter?
Same thing for aggregate stats.
Why do ox_put_raw() and ox_put_agg_raw() check for null stats
parameters? I don't think they're ever called with nulls.
I don't see value in ox_put_raw() and ox_put_agg_raw() returning byte
counts. It doesn't seem particularly useful here.
The !false test in oxs_flow_rem_stat_fields_pull() doesn't make sense:
if (error == OFPERR_OFPBMC_BAD_FIELD && !false) {
oxs_pull_flow_rem_stat_entry() doesn't seem to check that payload
lengths are correct.
In oxs_pull_flow_rem_stat_entry(), the following:
fr->duration_sec = ((uint32_t) ((duration_ &
0xFFFFFFFF00000000LL) >> 32));
fr->duration_nsec = ((uint32_t) (duration_ & 0xFFFFFFFFLL));
can be written as:
fr->duration_sec = duration >> 32;
fr->duration_nsec = duration;
Thanks,
Ben.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
