Re: [ovs-dev] [PATCH v2] ofp-monitor: Fixed the usage of 'usable_protocols' variable in 'parse_flow_monitor_request' function.

2019-11-01 Thread Ben Pfaff
On Tue, Jul 23, 2019 at 01:02:10PM -0700, Ashish Varma wrote:
> 'usable_protocols' is now getting set to OFPUTIL_P_OF10_ANY on return from
> 'parse_flow_monitor_request' function. The calling function now checks for the
> value in this variable against the 'allowed_protocols' variable.
> Also a check is added for a match field which is not supported in OpenFlow 1.0
> and return an error.
> Modified the man page of ovs-ofctl to reflect Flow Monitor support as
> OpenFlow 1.0 Nicira extension only.
> 
> Signed-off-by: Ashish Varma 
> ---
> v1-v2:
> Added a test that triggers the error condition fixed in this patch.
> Setting the usable_protocols variable to OFPUTIL_P_OF10_ANY or even a
> further subset if the match field requires it to. e.g. 'sctp_dst' is supported
> as only NXM protocol and not in STD protocol.
> Consistent wording for the error message on encountering an unusable flow
> format.

Thanks.  I applied this to master.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


[ovs-dev] [PATCH v2] ofp-monitor: Fixed the usage of 'usable_protocols' variable in 'parse_flow_monitor_request' function.

2019-07-23 Thread Ashish Varma
'usable_protocols' is now getting set to OFPUTIL_P_OF10_ANY on return from
'parse_flow_monitor_request' function. The calling function now checks for the
value in this variable against the 'allowed_protocols' variable.
Also a check is added for a match field which is not supported in OpenFlow 1.0
and return an error.
Modified the man page of ovs-ofctl to reflect Flow Monitor support as
OpenFlow 1.0 Nicira extension only.

Signed-off-by: Ashish Varma 
---
v1-v2:
Added a test that triggers the error condition fixed in this patch.
Setting the usable_protocols variable to OFPUTIL_P_OF10_ANY or even a
further subset if the match field requires it to. e.g. 'sctp_dst' is supported
as only NXM protocol and not in STD protocol.
Consistent wording for the error message on encountering an unusable flow
format.
---
 lib/ofp-monitor.c|  8 
 tests/ofproto.at | 21 +
 utilities/ovs-ofctl.8.in |  3 ++-
 utilities/ovs-ofctl.c|  8 
 4 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/lib/ofp-monitor.c b/lib/ofp-monitor.c
index aeebd7e..2fffb3b 100644
--- a/lib/ofp-monitor.c
+++ b/lib/ofp-monitor.c
@@ -469,6 +469,7 @@ parse_flow_monitor_request__(struct 
ofputil_flow_monitor_request *fmr,
 fmr->table_id = 0xff;
 match_init_catchall(>match);
 
+*usable_protocols = OFPUTIL_P_ANY;
 while (ofputil_parse_key_value(, , )) {
 const struct ofp_protocol *p;
 char *error = NULL;
@@ -493,6 +494,10 @@ parse_flow_monitor_request__(struct 
ofputil_flow_monitor_request *fmr,
 } else if (mf_from_name(name)) {
 error = ofp_parse_field(mf_from_name(name), value, port_map,
 >match, usable_protocols);
+if (!error && !(*usable_protocols & OFPUTIL_P_OF10_ANY)) {
+return xasprintf("%s: match field is not supported for "
+ "flow monitor", name);
+}
 } else {
 if (!*value) {
 return xasprintf("%s: field %s missing value", str_, name);
@@ -513,6 +518,9 @@ parse_flow_monitor_request__(struct 
ofputil_flow_monitor_request *fmr,
 if (error) {
 return error;
 }
+/* Flow Monitor is supported in OpenFlow 1.0 or can be further reduced
+ * to a few 1.0 flavors by a match field. */
+*usable_protocols &= OFPUTIL_P_OF10_ANY;
 }
 return NULL;
 }
diff --git a/tests/ofproto.at b/tests/ofproto.at
index a810dd6..298f932 100644
--- a/tests/ofproto.at
+++ b/tests/ofproto.at
@@ -5124,6 +5124,27 @@ NXT_FLOW_MONITOR_RESUMED:
 OVS_VSWITCHD_STOP
 AT_CLEANUP
 
+AT_SETUP([ofproto - flow monitoring usable protocols])
+AT_KEYWORDS([monitor])
+
+OVS_VSWITCHD_START
+
+on_exit 'kill `cat ovs-ofctl.pid`'
+ovs-ofctl -OOpenFlow14 monitor br0 watch:udp,udp_dst=8 --detach --no-chdir 
--pidfile >monitor.log 2>&1
+AT_CAPTURE_FILE([monitor.log])
+
+# ovs-ofctl should exit because monitor is not supported in OpenFlow 1.4
+OVS_WAIT_UNTIL([grep "ovs-ofctl: none of the usable flow formats 
(OpenFlow10,NXM) is among the allowed flow formats (OXM-OpenFlow14)" 
monitor.log])
+
+# check that only NXM flag is returned as usable protocols for sctp_dst
+# and ovs-ofctl should exit since monitor is not supported in OpenFlow 1.4
+ovs-ofctl -OOpenFlow14 monitor br0 watch:sctp,sctp_dst=9 --detach --no-chdir 
--pidfile >monitor.log 2>&1
+OVS_WAIT_UNTIL([grep "ovs-ofctl: none of the usable flow formats (NXM) is 
among the allowed flow formats (OXM-OpenFlow14)" monitor.log])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
+
 AT_SETUP([ofproto - event filtering (OpenFlow 1.3)])
 AT_KEYWORDS([monitor])
 OVS_VSWITCHD_START
diff --git a/utilities/ovs-ofctl.8.in b/utilities/ovs-ofctl.8.in
index 60b4a1c..cb5c612 100644
--- a/utilities/ovs-ofctl.8.in
+++ b/utilities/ovs-ofctl.8.in
@@ -578,7 +578,8 @@ monitoring will not show any traffic.
 .IP "\fBmonitor \fIswitch\fR [\fImiss-len\fR] [\fBinvalid_ttl\fR] 
[\fBwatch:\fR[\fIspec\fR...]]"
 Connects to \fIswitch\fR and prints to the console all OpenFlow
 messages received.  Usually, \fIswitch\fR should specify the name of a
-bridge in the \fBovs\-vswitchd\fR database.
+bridge in the \fBovs\-vswitchd\fR database. This is available only in
+OpenFlow 1.0 as Nicira extension.
 .IP
 If \fImiss-len\fR is provided, \fBovs\-ofctl\fR sends an OpenFlow ``set
 configuration'' message at connection setup time that requests
diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
index 754629d..c6f8111 100644
--- a/utilities/ovs-ofctl.c
+++ b/utilities/ovs-ofctl.c
@@ -2286,6 +2286,14 @@ ofctl_monitor(struct ovs_cmdl_context *ctx)
 ovs_fatal(0, "%s", error);
 }
 
+if (!(usable_protocols & allowed_protocols)) {
+char *allowed_s =
+ofputil_protocols_to_string(allowed_protocols);
+char *usable_s = ofputil_protocols_to_string(usable_protocols);
+ovs_fatal(0,