ovs-ofctl: Implement documented semantics of --flow-format for flow_mods.

Also adds a test and moves some code around in tests/ to make sure that
OFPROTO_START and OFPROTO_STOP are available in tests/ovs-ofctl.at.

Reported-by: Michael Mao <mmao@nicira.com>
Bug #4566.

diff --git a/tests/automake.mk b/tests/automake.mk
index 09ec23f..e2d3388 100644 (file)
--- a/tests/automake.mk
+++ b/tests/automake.mk
@@ -25,6 +25,7 @@ TESTSUITE_AT = \
        tests/timeval.at \
        tests/lockfile.at \
        tests/reconnect.at \
+       tests/ofproto-macros.at \
        tests/ofproto.at \
        tests/ovsdb.at \
        tests/ovsdb-log.at \

diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at
new file mode 100644 (file)
index 0000000..06f9b65
--- /dev/null
+++ b/tests/ofproto-macros.at
@@ -0,0 +1,16 @@
+m4_define([STRIP_XIDS], [[sed 's/ (xid=0x[0-9a-fA-F]*)//']])
+m4_define([STRIP_DURATION], [[sed 's/\bduration=[0-9.]*s/duration=?s/']])
+
+m4_define([OFPROTO_START],
+  [OVS_RUNDIR=$PWD; export OVS_RUNDIR
+   OVS_LOGDIR=$PWD; export OVS_LOGDIR
+   trap 'kill `cat ovs-openflowd.pid`' 0
+   AT_CAPTURE_FILE([ovs-openflowd.log])
+   AT_CHECK(
+     [ovs-openflowd --detach --pidfile --enable-dummy --log-file dummy@br0 none --datapath-id=fedcba9876543210 $1],
+     [0], [ignore], [ignore])
+])
+
+m4_define([OFPROTO_STOP],
+  [AT_CHECK([ovs-appctl -t ovs-openflowd exit])
+   trap '' 0])

diff --git a/tests/ofproto.at b/tests/ofproto.at
index a7dda06..f266f34 100644 (file)
--- a/tests/ofproto.at
+++ b/tests/ofproto.at
@@ -1,22 +1,5 @@
 AT_BANNER([ofproto])
 
-m4_define([STRIP_XIDS], [[sed 's/ (xid=0x[0-9a-fA-F]*)//']])
-m4_define([STRIP_DURATION], [[sed 's/\bduration=[0-9.]*s/duration=?s/']])
-
-m4_define([OFPROTO_START],
-  [OVS_RUNDIR=$PWD; export OVS_RUNDIR
-   OVS_LOGDIR=$PWD; export OVS_LOGDIR
-   trap 'kill `cat ovs-openflowd.pid`' 0
-   AT_CAPTURE_FILE([ovs-openflowd.log])
-   AT_CHECK(
-     [ovs-openflowd --detach --pidfile --enable-dummy --log-file dummy@br0 none --datapath-id=fedcba9876543210 $1],
-     [0], [ignore], [ignore])
-])
-
-m4_define([OFPROTO_STOP],
-  [AT_CHECK([ovs-appctl -t ovs-openflowd exit])
-   trap '' 0])
-
 AT_SETUP([ofproto - echo request])
 OFPROTO_START
 AT_CHECK([ovs-ofctl -vANY:ANY:WARN probe br0])

diff --git a/tests/ovs-ofctl.at b/tests/ovs-ofctl.at
index 111ed88..dfdebd3 100644 (file)
--- a/tests/ovs-ofctl.at
+++ b/tests/ovs-ofctl.at
@@ -464,3 +464,25 @@ nx_pull_match() returned error 44010101
 nx_pull_match() returned error 44010101
 ])
 AT_CLEANUP
+
+dnl Check that "-F openflow10" rejects a flow_mod with a tun_id, since
+dnl OpenFlow 1.0 doesn't support tunnels.
+AT_SETUP([ovs-ofctl -F option and tun_id])
+AT_CHECK([ovs-ofctl -F openflow10 add-flow dummy tun_id=123,actions=drop],
+  [1], [], [ovs-ofctl: flow cannot be expressed in flow format openflow10 (flow format tun_id_from_cookie or better is required)
+])
+AT_CLEANUP
+
+dnl Check that "-F nxm" really forces add-flow to use the NXM flow format.
+dnl (If it doesn't, then either the tun_id won't show up at all, or it will
+dnl additionally show up as the top 32 bits of the cookie.)  This checks
+dnl for regression against bug #4566.
+AT_SETUP([ovs-ofctl -F option with flow_mods])
+OFPROTO_START
+AT_CHECK([ovs-ofctl -F nxm add-flow br0 tun_id=0x12345678,actions=drop])
+AT_CHECK([ovs-ofctl dump-flows br0 | STRIP_XIDS | STRIP_DURATION], [0], [dnl
+NXST_FLOW reply:
+ cookie=0x0, duration=?s, table_id=0, n_packets=0, n_bytes=0, tun_id=0x12345678 actions=drop
+])
+OFPROTO_STOP
+AT_CLEANUP

diff --git a/tests/testsuite.at b/tests/testsuite.at
index d41f504..5625f8b 100644 (file)
--- a/tests/testsuite.at
+++ b/tests/testsuite.at
@@ -1,6 +1,6 @@
 AT_INIT
 
-AT_COPYRIGHT([Copyright (c) 2009, 2010 Nicira Networks.
+AT_COPYRIGHT([Copyright (c) 2009, 2010, 2011 Nicira Networks.
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.
@@ -36,6 +36,7 @@ m4_define([OVS_WAIT_UNTIL], [OVS_WAIT([if $1; then exit 0; fi], [$2])])
 m4_define([OVS_WAIT_WHILE], [OVS_WAIT([if $1; then :; else exit 0; fi], [$2])])
 
 m4_include([tests/ovsdb-macros.at])
+m4_include([tests/ofproto-macros.at])
 
 m4_include([tests/library.at])
 m4_include([tests/classifier.at])

diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
index 9f5a0a5..a96b77a 100644 (file)
--- a/utilities/ovs-ofctl.c
+++ b/utilities/ovs-ofctl.c
@@ -622,6 +622,43 @@ do_queue_stats(int argc, char *argv[])
     dump_stats_transaction(argv[1], request);
 }
 
+/* Sets up the flow format for a vconn that will be used to modify the flow
+ * table.  Returns the flow format used, after possibly adding an OpenFlow
+ * request to 'requests'.
+ *
+ * If 'preferred_flow_format' is -1, returns NXFF_OPENFLOW10 without modifying
+ * 'requests', since NXFF_OPENFLOW10 is the default flow format for any
+ * OpenFlow connection.
+ *
+ * If 'preferred_flow_format' is a specific format, adds a request to set that
+ * format to 'requests' and returns the format. */
+static enum nx_flow_format
+set_initial_format_for_flow_mod(struct list *requests)
+{
+    if (preferred_flow_format < 0) {
+        return NXFF_OPENFLOW10;
+    } else {
+        struct ofpbuf *sff;
+
+        sff = ofputil_make_set_flow_format(preferred_flow_format);
+        list_push_back(requests, &sff->list_node);
+        return preferred_flow_format;
+    }
+}
+
+/* Checks that 'flow_format' is acceptable as a flow format after a flow_mod
+ * operation, given the global 'preferred_flow_format'. */
+static void
+check_final_format_for_flow_mod(enum nx_flow_format flow_format)
+{
+    if (preferred_flow_format >= 0 && flow_format != preferred_flow_format) {
+        ovs_fatal(0, "flow cannot be expressed in flow format %s "
+                  "(flow format %s or better is required)",
+                  ofputil_flow_format_to_string(preferred_flow_format),
+                  ofputil_flow_format_to_string(flow_format));
+    }
+}
+
 static void
 do_flow_mod__(int argc OVS_UNUSED, char *argv[], uint16_t command)
 {
@@ -630,9 +667,11 @@ do_flow_mod__(int argc OVS_UNUSED, char *argv[], uint16_t command)
     struct vconn *vconn;
 
     list_init(&requests);
-    flow_format = NXFF_OPENFLOW10;
+    flow_format = set_initial_format_for_flow_mod(&requests);
+
     parse_ofp_flow_mod_str(&requests, &flow_format, argc > 2 ? argv[2] : "",
                            command);
+    check_final_format_for_flow_mod(flow_format);
 
     open_vconn(argv[1], &vconn);
     transact_multiple_noreply(vconn, &requests);
@@ -659,10 +698,11 @@ do_add_flows(int argc OVS_UNUSED, char *argv[])
     }
 
     list_init(&requests);
-    flow_format = NXFF_OPENFLOW10;
+    flow_format = set_initial_format_for_flow_mod(&requests);
 
     open_vconn(argv[1], &vconn);
     while (parse_ofp_add_flow_file(&requests, &flow_format, file)) {
+        check_final_format_for_flow_mod(flow_format);
         transact_multiple_noreply(vconn, &requests);
     }
     vconn_close(vconn);