[ovs-dev] [PATCH 2/2] ovs-ofctl: Warn about flows not in normal form.

[Ben Pfaff]

Lots of people get this wrong.

Bug #185.
---
 utilities/ovs-ofctl.8.in |   28 ++++++++++++++++++++--------
 utilities/ovs-ofctl.c    |   13 +++++++++++++
 2 files changed, 33 insertions(+), 8 deletions(-)

diff --git a/utilities/ovs-ofctl.8.in b/utilities/ovs-ofctl.8.in
index 8d48d98..d0a598c 100644
--- a/utilities/ovs-ofctl.8.in
+++ b/utilities/ovs-ofctl.8.in
@@ -223,6 +223,17 @@ space.  (Embedding spaces into a flow description normally requires
 quoting to prevent the shell from breaking the description into
 multiple arguments.)
 .PP
+Flow descriptions should be in \fBnormal form\fR.  This means that a
+flow may only specify a value for an L3 field if it also specifies a
+particular L2 protocol, and that a flow may only specify an L4 field
+if it also specifies particular L2 and L3 protocol types.  For
+example, if the L2 protocol type \fBdl_type\fR is wildcarded, then L3
+fields \fBnw_src\fR, \fBnw_dst\fR, and \fBnw_proto\fR must also be
+wildcarded.  Similarly, if \fBdl_type\fR or \fBnw_proto\fR (the L3
+protocol type) is wildcarded, so must be \fBtp_dst\fR and
+\fBtp_src\fR, which are L4 fields.  \fBovs\-ofctl\fR will warn about
+flows not in normal form.
+.PP
 The following field assignments describe how a flow matches a packet.
 If any of these assignments is omitted from the flow syntax, the field
 is treated as a wildcard; thus, if all of them are omitted, the
@@ -273,8 +284,8 @@ When \fBdl_type=0x0806\fR or \fBarp\fR is specified, matches the
 IPv4 and Ethernet.
 .IP
 When \fBdl_type\fR is wildcarded or set to a value other than 0x0800
-or 0x0806, the values of \fBnw_src\fR and \fBnw_dst\fR are silently
-ignored.
+or 0x0806, the values of \fBnw_src\fR and \fBnw_dst\fR are ignored
+(see \fBFlow Syntax\fR above).
 .
 .IP \fBnw_proto=\fIproto\fR
 When \fBip\fR or \fBdl_type=0x0800\fR is specified, matches IP
@@ -286,16 +297,17 @@ When \fBarp\fR or \fBdl_type=0x0806\fR is specified, matches the lower
 0.
 .IP
 When \fBdl_type\fR is wildcarded or set to a value other than 0x0800
-or 0x0806, the value of \fBnw_proto\fR is silently ignored.
+or 0x0806, the value of \fBnw_proto\fR is ignored (see \fBFlow
+Syntax\fR above).
 .
 .IP \fBnw_tos=\fItos\fR
 Matches IP ToS/DSCP field \fItos\fR, which is specified as a decimal 
 number between 0 and 255, inclusive.  Note that the two lower reserved
 bits are ignored for matching purposes.
 .IP
-The value of \fBnw_proto\fR is silently ignored unless
-\fBdl_type=0x0800\fR, \fBip\fR, \fBicmp\fR, \fBtcp\fR, or \fBudp\fR is
-also specified.
+The value of \fBnw_proto\fR is ignored unless \fBdl_type=0x0800\fR,
+\fBip\fR, \fBicmp\fR, \fBtcp\fR, or \fBudp\fR is also specified (see
+\fBFlow Syntax\fR above).
 .
 .IP \fBtp_src=\fIport\fR
 .IQ \fBtp_dst=\fIport\fR
@@ -306,7 +318,7 @@ between 0 and 65535, inclusive (e.g. 80 to match packets originating
 from a HTTP server).
 .IP
 When \fBdl_type\fR and \fBnw_proto\fR take other values, the values of
-these settings are silently ignored.
+these settings are ignored (see \fBFlow Syntax\fR above).
 .
 .IP \fBicmp_type=\fItype\fR
 .IQ \fBicmp_code=\fIcode\fR
@@ -315,7 +327,7 @@ the ICMP type and \fIcode\fR matches the ICMP code.  Each is specified
 as a decimal number between 0 and 255, inclusive.
 .IP
 When \fBdl_type\fR and \fBnw_proto\fR take other values, the values of
-these settings are silently ignored.
+these settings are ignored (see \fBFlow Syntax\fR above).
 .
 .PP
 The following shorthand notations are also available:
diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
index c2f4fef..c0b7628 100644
--- a/utilities/ovs-ofctl.c
+++ b/utilities/ovs-ofctl.c
@@ -769,6 +769,7 @@ str_to_flow(char *string, struct ofp_match *match, struct ofpbuf *actions,
             uint16_t *idle_timeout, uint16_t *hard_timeout, 
             uint64_t *cookie)
 {
+    struct ofp_match normalized;
     char *save_ptr = NULL;
     char *name;
     uint32_t wildcards;
@@ -870,6 +871,18 @@ str_to_flow(char *string, struct ofp_match *match, struct ofpbuf *actions,
         }
     }
     match->wildcards = htonl(wildcards);
+
+    normalized = *match;
+    normalize_match(&normalized);
+    if (memcmp(match, &normalized, sizeof normalized)) {
+        char *old = ofp_match_to_literal_string(match);
+        char *new = ofp_match_to_literal_string(&normalized);
+        VLOG_WARN("The specified flow is not in normal form:");
+        VLOG_WARN(" as specified: %s", old);
+        VLOG_WARN("as normalized: %s", new);
+        free(old);
+        free(new);
+    }
 }
 
 static void
-- 
1.7.1