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 <[email protected]>
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
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
