connmgr: Only send role status messages to OpenFlow 1.4+ controllers.

Only OpenFlow 1.4 and later support role status messages, but this code
tried to send them to all controllers, which caused an assertion failure.

Also, add tests to check that role status messages work, and that they
don't cause trouble with OF1.2.

Reported-by: Anup Khadka <khadka.py@gmail.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>
Acked-by: Justin Pettit <jpettit@nicira.com>

diff --git a/AUTHORS b/AUTHORS
index d7123e0..5723b93 100644 (file)
--- a/AUTHORS
+++ b/AUTHORS
@@ -155,6 +155,7 @@ Andreas Beckmann        debian@abeckmann.de
 Andrei Andone           andrei.andone@softvision.ro
 Anshuman Manral         anshuman.manral@outlook.com
 Anton Matsiuk           anton.matsiuk@gmail.com
+Anup Khadka             khadka.py@gmail.com
 Anuprem Chalvadi        achalvadi@vmware.com
 Ariel Tubaltsev         atubaltsev@vmware.com
 Atzm Watanabe           atzm@stratosphere.co.jp

diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index b303653..1b4b29e 100644 (file)
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -4969,22 +4969,31 @@ ofputil_encode_role_reply(const struct ofp_header *request,
     return buf;
 }
 \f
+/* Encodes "role status" message 'status' for sending in the given
+ * 'protocol'.  Returns the role status message, if 'protocol' supports them,
+ * otherwise a null pointer. */
 struct ofpbuf *
 ofputil_encode_role_status(const struct ofputil_role_status *status,
                            enum ofputil_protocol protocol)
 {
-    struct ofpbuf *buf;
     enum ofp_version version;
-    struct ofp14_role_status *rstatus;
 
     version = ofputil_protocol_to_ofp_version(protocol);
-    buf = ofpraw_alloc_xid(OFPRAW_OFPT14_ROLE_STATUS, version, htonl(0), 0);
-    rstatus = ofpbuf_put_zeros(buf, sizeof *rstatus);
-    rstatus->role = htonl(status->role);
-    rstatus->reason = status->reason;
-    rstatus->generation_id = htonll(status->generation_id);
-
-    return buf;
+    if (version >= OFP14_VERSION) {
+        struct ofp14_role_status *rstatus;
+        struct ofpbuf *buf;
+
+        buf = ofpraw_alloc_xid(OFPRAW_OFPT14_ROLE_STATUS, version, htonl(0),
+                               0);
+        rstatus = ofpbuf_put_zeros(buf, sizeof *rstatus);
+        rstatus->role = htonl(status->role);
+        rstatus->reason = status->reason;
+        rstatus->generation_id = htonll(status->generation_id);
+
+        return buf;
+    } else {
+        return NULL;
+    }
 }
 
 enum ofperr

diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c
index 9ca6b04..489ca14 100644 (file)
--- a/ofproto/connmgr.c
+++ b/ofproto/connmgr.c
@@ -935,8 +935,9 @@ ofconn_send_role_status(struct ofconn *ofconn, uint32_t role, uint8_t reason)
     ofconn_get_master_election_id(ofconn, &status.generation_id);
 
     buf = ofputil_encode_role_status(&status, ofconn_get_protocol(ofconn));
-
-    ofconn_send(ofconn, buf, NULL);
+    if (buf) {
+        ofconn_send(ofconn, buf, NULL);
+    }
 }
 
 /* Changes 'ofconn''s role to 'role'.  If 'role' is OFPCR12_ROLE_MASTER then

diff --git a/tests/ofproto.at b/tests/ofproto.at
index 7df73de..f14cbcc 100644 (file)
--- a/tests/ofproto.at
+++ b/tests/ofproto.at
@@ -1865,41 +1865,128 @@ dnl This test checks that the role request/response messaging works
 dnl and that generation_id is handled properly.
 AT_SETUP([ofproto - controller role (OpenFlow 1.2)])
 OVS_VSWITCHD_START
-AT_CHECK([ovs-ofctl -O OpenFlow12 monitor br0 --detach --no-chdir --pidfile])
-
-ovs-appctl -t ovs-ofctl ofctl/barrier
-ovs-appctl -t ovs-ofctl ofctl/set-output-file monitor.log
-: > expout
-: > experr
+ON_EXIT([kill `cat c1.pid c2.pid`])
+
+# Start two ovs-ofctl controller processes.
+AT_CAPTURE_FILE([monitor1.log])
+AT_CAPTURE_FILE([expout1])
+AT_CAPTURE_FILE([experr1])
+AT_CAPTURE_FILE([monitor2.log])
+AT_CAPTURE_FILE([expout2])
+AT_CAPTURE_FILE([experr2])
+for i in 1 2; do
+     AT_CHECK([ovs-ofctl -O OpenFlow12 monitor br0 --detach --no-chdir --pidfile=`pwd`/c$i.pid --unixctl=`pwd`/c$i])
+    ovs-appctl -t `pwd`/c$i ofctl/barrier
+    ovs-appctl -t `pwd`/c$i ofctl/set-output-file monitor$i.log
+    : > expout$i
+    : > experr$i
+
+    # find out current role
+    ovs-appctl -t `pwd`/c$i ofctl/send 031800180000000200000000000000000000000000000000
+    echo >>experr$i "send: OFPT_ROLE_REQUEST (OF1.2): role=nochange"
+    echo >>expout$i "OFPT_ROLE_REPLY (OF1.2): role=equal"
+done
 
-# find out current role
-ovs-appctl -t ovs-ofctl ofctl/send 031800180000000200000000000000000000000000000000
-echo >>experr "send: OFPT_ROLE_REQUEST (OF1.2) (xid=0x2): role=nochange"
-echo >>expout "OFPT_ROLE_REPLY (OF1.2) (xid=0x2): role=equal"
+# controller 1: Become slave (generation_id is initially undefined, so
+# 2^63+2 should not be stale)
+ovs-appctl -t `pwd`/c1 ofctl/send 031800180000000300000003000000008000000000000002
+echo >>experr1 "send: OFPT_ROLE_REQUEST (OF1.2): role=slave generation_id=9223372036854775810"
+echo >>expout1 "OFPT_ROLE_REPLY (OF1.2): role=slave generation_id=9223372036854775810"
+
+# controller 2: Become master.
+ovs-appctl -t `pwd`/c2 ofctl/send 031800180000000300000002000000008000000000000003
+echo >>experr2 "send: OFPT_ROLE_REQUEST (OF1.2): role=master generation_id=9223372036854775811"
+echo >>expout2 "OFPT_ROLE_REPLY (OF1.2): role=master generation_id=9223372036854775811"
+
+# controller 1: Try to become the master using a stale generation ID
+ovs-appctl -t `pwd`/c1 ofctl/send 031800180000000400000002000000000000000000000003
+echo >>experr1 "send: OFPT_ROLE_REQUEST (OF1.2): role=master generation_id=3"
+echo >>expout1 "OFPT_ERROR (OF1.2): OFPRRFC_STALE"
+echo >>expout1 "OFPT_ROLE_REQUEST (OF1.2): role=master generation_id=3"
+
+# controller 1: Become master using a valid generation ID
+ovs-appctl -t `pwd`/c1 ofctl/send 031800180000000500000002000000000000000000000001
+echo >>experr1 "send: OFPT_ROLE_REQUEST (OF1.2): role=master generation_id=1"
+echo >>expout1 "OFPT_ROLE_REPLY (OF1.2): role=master generation_id=1"
+
+for i in 1 2; do
+    ovs-appctl -t `pwd`/c$i ofctl/barrier
+    echo >>expout$i "OFPT_BARRIER_REPLY (OF1.2):"
+done
 
-# Become slave (generation_id is initially undefined, so 2^63+2 should not be stale)
-ovs-appctl -t ovs-ofctl ofctl/send 031800180000000300000003000000008000000000000002
-echo >>experr "send: OFPT_ROLE_REQUEST (OF1.2) (xid=0x3): role=slave generation_id=9223372036854775810"
-echo >>expout "OFPT_ROLE_REPLY (OF1.2) (xid=0x3): role=slave generation_id=9223372036854775810"
+# Check output.
+for i in 1 2; do
+    cp expout$i expout
+    AT_CHECK([grep -v '^send:' monitor$i.log | STRIP_XIDS], [0], [expout])
+    cp experr$i expout
+    AT_CHECK([grep '^send:' monitor$i.log | STRIP_XIDS], [0], [expout])
+done
+OVS_VSWITCHD_STOP
+AT_CLEANUP
 
-# Try to become the master using a stale generation ID
-ovs-appctl -t ovs-ofctl ofctl/send 031800180000000400000002000000000000000000000002
-echo >>experr "send: OFPT_ROLE_REQUEST (OF1.2) (xid=0x4): role=master generation_id=2"
-echo >>expout "OFPT_ERROR (OF1.2) (xid=0x4): OFPRRFC_STALE"
-echo >>expout "OFPT_ROLE_REQUEST (OF1.2) (xid=0x4): role=master generation_id=2"
-
-# Become master using a valid generation ID
-ovs-appctl -t ovs-ofctl ofctl/send 031800180000000500000002000000000000000000000001
-echo >>experr "send: OFPT_ROLE_REQUEST (OF1.2) (xid=0x5): role=master generation_id=1"
-echo >>expout "OFPT_ROLE_REPLY (OF1.2) (xid=0x5): role=master generation_id=1"
-ovs-appctl -t ovs-ofctl ofctl/barrier
-echo >>expout "OFPT_BARRIER_REPLY (OF1.2) (xid=0x3):"
+dnl This test checks that the role request/response messaging works,
+dnl that generation_id is handled properly, and that role status update
+dnl messages are sent when a controller's role gets changed from master
+dnl to slave.
+AT_SETUP([ofproto - controller role (OpenFlow 1.4)])
+OVS_VSWITCHD_START
+ON_EXIT([kill `cat c1.pid c2.pid`])
+
+# Start two ovs-ofctl controller processes.
+AT_CAPTURE_FILE([monitor1.log])
+AT_CAPTURE_FILE([expout1])
+AT_CAPTURE_FILE([experr1])
+AT_CAPTURE_FILE([monitor2.log])
+AT_CAPTURE_FILE([expout2])
+AT_CAPTURE_FILE([experr2])
+for i in 1 2; do
+     AT_CHECK([ovs-ofctl -O OpenFlow14 monitor br0 --detach --no-chdir --pidfile=`pwd`/c$i.pid --unixctl=`pwd`/c$i])
+    ovs-appctl -t `pwd`/c$i ofctl/barrier
+    ovs-appctl -t `pwd`/c$i ofctl/set-output-file monitor$i.log
+    : > expout$i
+    : > experr$i
+
+    # find out current role
+    ovs-appctl -t `pwd`/c$i ofctl/send 051800180000000200000000000000000000000000000000
+    echo >>experr$i "send: OFPT_ROLE_REQUEST (OF1.4): role=nochange"
+    echo >>expout$i "OFPT_ROLE_REPLY (OF1.4): role=equal"
+done
 
-AT_CHECK([grep -v '^send:' monitor.log], [0], [expout])
-mv experr expout
-AT_CHECK([grep '^send:' monitor.log], [0], [expout])
+# controller 1: Become slave (generation_id is initially undefined, so
+# 2^63+2 should not be stale)
+ovs-appctl -t `pwd`/c1 ofctl/send 051800180000000300000003000000008000000000000002
+echo >>experr1 "send: OFPT_ROLE_REQUEST (OF1.4): role=slave generation_id=9223372036854775810"
+echo >>expout1 "OFPT_ROLE_REPLY (OF1.4): role=slave generation_id=9223372036854775810"
+
+# controller 2: Become master.
+ovs-appctl -t `pwd`/c2 ofctl/send 051800180000000300000002000000008000000000000003
+echo >>experr2 "send: OFPT_ROLE_REQUEST (OF1.4): role=master generation_id=9223372036854775811"
+echo >>expout2 "OFPT_ROLE_REPLY (OF1.4): role=master generation_id=9223372036854775811"
+
+# controller 1: Try to become the master using a stale generation ID
+ovs-appctl -t `pwd`/c1 ofctl/send 051800180000000400000002000000000000000000000003
+echo >>experr1 "send: OFPT_ROLE_REQUEST (OF1.4): role=master generation_id=3"
+echo >>expout1 "OFPT_ERROR (OF1.4): OFPRRFC_STALE"
+echo >>expout1 "OFPT_ROLE_REQUEST (OF1.4): role=master generation_id=3"
+
+# controller 1: Become master using a valid generation ID
+ovs-appctl -t `pwd`/c1 ofctl/send 051800180000000500000002000000000000000000000001
+echo >>experr1 "send: OFPT_ROLE_REQUEST (OF1.4): role=master generation_id=1"
+echo >>expout1 "OFPT_ROLE_REPLY (OF1.4): role=master generation_id=1"
+echo >>expout2 "OFPT_ROLE_STATUS (OF1.4): role=slave generation_id=1 reason=master_request"
+
+for i in 1 2; do
+    ovs-appctl -t `pwd`/c$i ofctl/barrier
+    echo >>expout$i "OFPT_BARRIER_REPLY (OF1.4):"
+done
 
-ovs-appctl -t ovs-ofctl exit
+# Check output.
+for i in 1 2; do
+    cp expout$i expout
+    AT_CHECK([grep -v '^send:' monitor$i.log | STRIP_XIDS], [0], [expout])
+    cp experr$i expout
+    AT_CHECK([grep '^send:' monitor$i.log | STRIP_XIDS], [0], [expout])
+done
 OVS_VSWITCHD_STOP
 AT_CLEANUP