ofproto-dpif: Accept OpenFlow-like flows in "ofproto/trace".

Until now it has not been possible to directly trace flows that include
register values and other concepts that are not in datapath flows, because
"ofproto/trace" requires a flow in the format output by "ovs-dpctl
dump-flows", which doesn't know anything about registers.  This commit
makes it possible to instead specify an OpenFlow-like flow.

Feature #10084.
Requested-by: Igor Ganichev <iganichev@nicira.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>

diff --git a/AUTHORS b/AUTHORS
index b4ec9c7..e207f3c 100644 (file)
--- a/AUTHORS
+++ b/AUTHORS
@@ -101,6 +101,7 @@ Hassan Khan             hassan.khan@seecs.edu.pk
 Hector Oron             hector.oron@gmail.com
 Henrik Amren            henrik@nicira.com
 Hiroshi Tanaka          htanaka@nicira.com
+Igor Ganichev           iganichev@nicira.com
 Jacob Cherkas           jcherkas@nicira.com
 Jad Naous               jnaous@gmail.com
 Jamal Hadi Salim        hadi@cyberus.ca

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 8a73209..11bde38 100644 (file)
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -42,6 +42,7 @@
 #include "odp-util.h"
 #include "ofp-util.h"
 #include "ofpbuf.h"
+#include "ofp-parse.h"
 #include "ofp-print.h"
 #include "ofproto-dpif-governor.h"
 #include "ofproto-dpif-sflow.h"
@@ -6387,23 +6388,48 @@ ofproto_unixctl_trace(struct unixctl_conn *conn, int argc, const char *argv[],
         /* ofproto/trace dpname flow [-generate] */
         const char *flow_s = argv[2];
         const char *generate_s = argv[3];
-        int error;
 
-        /* Convert string to datapath key. */
-        ofpbuf_init(&odp_key, 0);
-        error = odp_flow_key_from_string(flow_s, NULL, &odp_key);
-        if (error) {
-            unixctl_command_reply_error(conn, "Bad flow syntax");
-            goto exit;
-        }
+        /* Allow 'flow_s' to be either a datapath flow or an OpenFlow-like
+         * flow.  We guess which type it is based on whether 'flow_s' contains
+         * an '(', since a datapath flow always contains '(') but an
+         * OpenFlow-like flow should not (in fact it's allowed but I believe
+         * that's not documented anywhere).
+         *
+         * An alternative would be to try to parse 'flow_s' both ways, but then
+         * it would be tricky giving a sensible error message.  After all, do
+         * you just say "syntax error" or do you present both error messages?
+         * Both choices seem lousy. */
+        if (strchr(flow_s, '(')) {
+            int error;
+
+            /* Convert string to datapath key. */
+            ofpbuf_init(&odp_key, 0);
+            error = odp_flow_key_from_string(flow_s, NULL, &odp_key);
+            if (error) {
+                unixctl_command_reply_error(conn, "Bad flow syntax");
+                goto exit;
+            }
 
-        /* Convert odp_key to flow. */
-        error = ofproto_dpif_extract_flow_key(ofproto, odp_key.data,
-                                              odp_key.size, &flow,
-                                              &initial_tci, NULL);
-        if (error == ODP_FIT_ERROR) {
-            unixctl_command_reply_error(conn, "Invalid flow");
-            goto exit;
+            /* Convert odp_key to flow. */
+            error = ofproto_dpif_extract_flow_key(ofproto, odp_key.data,
+                                                  odp_key.size, &flow,
+                                                  &initial_tci, NULL);
+            if (error == ODP_FIT_ERROR) {
+                unixctl_command_reply_error(conn, "Invalid flow");
+                goto exit;
+            }
+        } else {
+            char *error_s;
+
+            error_s = parse_ofp_exact_flow(&flow, argv[2]);
+            if (error_s) {
+                unixctl_command_reply_error(conn, error_s);
+                free(error_s);
+                goto exit;
+            }
+
+            initial_tci = flow.vlan_tci;
+            vsp_adjust_flow(ofproto, &flow);
         }
 
         /* Generate a packet, if requested. */

diff --git a/ofproto/ofproto-unixctl.man b/ofproto/ofproto-unixctl.man
index ad9e02d..9d0c8e3 100644 (file)
--- a/ofproto/ofproto-unixctl.man
+++ b/ofproto/ofproto-unixctl.man
@@ -7,7 +7,7 @@ Lists the names of the running ofproto instances.  These are the names
 that may be used on \fBofproto/trace\fR.
 .
 .IP "\fBofproto/trace \fIswitch priority tun_id in_port packet\fR"
-.IQ "\fBofproto/trace \fIswitch odp_flow \fB\-generate\fR"
+.IQ "\fBofproto/trace \fIswitch flow \fB\-generate\fR"
 Traces the path of an imaginary packet through \fIswitch\fR.  Both
 forms require \fIswitch\fR, the switch on which the packet arrived
 (one of those listed by \fBofproto/list\fR).  The first form specifies
@@ -31,32 +31,36 @@ utilities provide easier ways.
 .IP
 The second form specifies the packet's contents implicitly:
 .RS
-.IP "\fIodp_flow\fR"
-A flow in the form printed by \fBovs\-dpctl\fR(8)'s \fBdump\-flows\fR
+.IP "\flow\fR"
+A flow in one of two forms: either the form printed by
+\fBovs\-dpctl\fR(8)'s \fBdump\-flows\fR command, or in a format
+similar to that accepted by \Bovs\-ofctl\fR(8)'s \fBadd\-flow\fR
 command.  This is not an OpenFlow flow: besides other differences, it
 never contains wildcards.  \fB\*(PN\fR generates an arbitrary packet
-that has the specified \fIodp_flow\fR.
+that has the specified \flow\fR.
 .RE
 .IP
 \fB\*(PN\fR will respond with extensive information on how the packet
 would be handled if it were to be received.  The packet will not
 actually be sent, but side effects such as MAC learning will occur.
 .
-.IP "\fBofproto/trace \fIswitch odp_flow\fR"
+.IP "\fBofproto/trace \fIswitch flow\fR"
 Traces the path of a packet in an imaginary flow through
 \fIswitch\fR.  The arguments are:
 .RS
 .IP "\fIswitch\fR"
 The switch on which the packet arrived (one of those listed by
 \fBofproto/list\fR).
-.IP "\fIodp_flow\fR"
-A flow in the form printed by \fBovs\-dpctl\fR(8)'s \fBdump\-flows\fR
+.IP "\flow\fR"
+A flow in one of two forms: either the form printed by
+\fBovs\-dpctl\fR(8)'s \fBdump\-flows\fR command, or in a format
+similar to that accepted by \Bovs\-ofctl\fR(8)'s \fBadd\-flow\fR
 command.  This is not an OpenFlow flow: besides other differences, it
 never contains wildcards.
 .RE
 .IP
 \fB\*(PN\fR will respond with extensive information on how a packet
-in \fIodp_flow\fR would be handled if it were received by
+in \flow\fR would be handled if it were received by
 \fIswitch\fR.  No packet will actually be sent.  Some side effects may
 occur, but MAC learning in particular will not.
 .IP

diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 2c5df96..17b064d 100644 (file)
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -11,7 +11,7 @@ table=1 in_port=2 priority=1500 icmp actions=output(17),resubmit(,2)
 table=1 in_port=3 priority=1500 icmp actions=output(14),resubmit(,2)
 ])
 AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
-AT_CHECK([ovs-appctl ofproto/trace br0 'in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=128,frag=no),icmp(type=8,code=0)'], [0], [stdout])
+AT_CHECK([ovs-appctl ofproto/trace br0 'in_port=1,dl_src=50:54:00:00:00:05,dl_dst=50:54:00:00:00:07,dl_type=0x0800,nw_src=192.168.0.1,nw_dst=192.168.0.2,nw_proto=1,nw_tos=0,nw_ttl=128,icmp_type=8,icmp_code=0'], [0], [stdout])
 AT_CHECK([tail -1 stdout], [0],
   [Datapath actions: 10,11,12,13,14,15,16,17,18,19,20,21
 ])