Thanks for the feedback. Would revert back shortly with replies and few questions followed by the v2 patch.
On Mon, Dec 10, 2018 at 1:54 PM Ben Pfaff <b...@ovn.org> wrote: > On Wed, Nov 28, 2018 at 10:30:07AM -0800, Ashish Varma wrote: > > OVS supports Nicira version of Flow Monitor feature which allows an > OpenFlow > > controller to keep track of any changes in the flow table. (The changes > can > > done by the controller itself or by any other controller connected to > OVS.) > > This patch adds support for the OpenFlow 1.4+ OFPMP_FLOW_MONITOR > multipart > > message. > > Also added support in "ovs-ofctl monitor" to send OpenFlow 1.4+ messages > to > > OVS. > > Added unit test cases to test the OpenFlow version of Flow Monitor > messages. > > > > Signed-off-by: Ashish Varma <ashishvarma....@gmail.com> > > Thanks a lot for working on this. The OF1.4+ flow monitor support was > based on the OVS design for the same feature, so it's a shame that we > didn't implement it early on. > > In openflow-1.4.h, please align the comments on members of structs and > enumerations to some common column, as is the usual practice in OVS > code, e.g. instead of this: > > /* struct ofp14_flow_update_full. */ > OFPFME_INITIAL = 0, /* Flow present when flow monitor created. */ > OFPFME_ADDED = 1, /* Flow was added. */ > OFPFME_REMOVED = 2, /* Flow was removed. */ > OFPFME_MODIFIED = 3, /* Flow instructions were changed. */ > /* struct ofp14_flow_update_abbrev. */ > OFPFME_ABBREV = 4, /* Abbreviated reply. */ > /* struct ofp14_flow_update_header. */ > OFPFME_PAUSED = 5, /* Monitoring paused (out of buffer space). */ > OFPFME_RESUMED = 6, /* Monitoring resumed. */ > > more like this (also adding some blank lines for clarity): > > /* struct ofp14_flow_update_full. */ > OFPFME_INITIAL = 0, /* Flow present when flow monitor created. > */ > OFPFME_ADDED = 1, /* Flow was added. */ > OFPFME_REMOVED = 2, /* Flow was removed. */ > OFPFME_MODIFIED = 3, /* Flow instructions were changed. */ > > /* struct ofp14_flow_update_abbrev. */ > OFPFME_ABBREV = 4, /* Abbreviated reply. */ > > /* struct ofp14_flow_update_header. */ > OFPFME_PAUSED = 5, /* Monitoring paused (out of buffer > space). */ > OFPFME_RESUMED = 6, /* Monitoring resumed. */ > > Also please use the "14" infix for these enumeration members, > e.g. OFPFME14_INITIAL. > > The OFPUTIL_FMF_* flags aren't really abstracted from NXFMF_* and > OFPFMF14_*; they're just a copy of the OFPFMF14_* ones with the names > changed. I think it might be better to just use OFPFMF14_* with a note > that NXFMF_* doesn't have NXFMF_ONLY_OWN. > > It's usually unnecessary to use a union of an ofp_port_t and an > ofp11_port_t, because OVS can usually reject unsupported ports when it > parses them from OpenFlow using ofputil_port_from_ofp11(). Maybe there > is some exception here but that's not obvious to me yet. > > I don't see how a piece of code that looks at an > ofputil_flow_monitor_request can tell whether to look at > out_port.ofp_port or out_port.ofp11_port. > > Our usual style is to start a comment with a capital letter and end it > with a period: > > /* there is one to one mapping between ofp14_flow_update_event and > * ofputil_flow_update_event */ > > The name ofputil_get_util_flow_update_event_from_nx_event() is pretty > long and I don't know what the _util_ part of it is there for. > > In new code, please try to declare (and initialize) variables at their > first use, when possible, rather than the older style we used of > declaring variables at the top of a block. > > I think that ofputil_start_flow_update() should always use > NXST_FLOW_MONITOR_REPLY for OpenFlow 1.0, so it seems to me that it > should check for OFPUTIL_P_OF10_ANY rather than OFPUTIL_P_OF10_STD_ANY. > (Or it could accept an OpenFlow version instead of a protocol.) > > I don't understand why ofputil_append_flow_update() checks for > OFPUTIL_P_OF10_STD in particular. It seems like any OF1.0 protocol > would make sense. (Or it could accept an OpenFlow version instead of a > protocol.) > > In general I'm not sure of the rationale here for how OpenFlow versions > are handled. Currently, it looks like OVS supports flow monitoring in > OF1.0 only. The OpenFlow specification defines flow monitoring for > OF1.4 and later and for OF1.3 as an "official extension" in extensions > pack 2 (which you can find at > > https://www.opennetworking.org/wp-content/uploads/2014/10/openflow-extensions-1.3.x-pack2.zip > ). > So we should use the official specs for 1.3 and later. For 1.1 and 1.2, > we have a choice of supporting our extension or the OpenFlow version. I > don't have a particular preference. > > Thanks! I'll look forward to the next revision. > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
