controller/ofp_handler: improve version negotiation

- fix string concatenation. Needs surrounding paren.
- more version checks
- make hello_failed private. (adding "_" prefix)

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>

1 files changed, 71 insertions, 19 deletions

diff --git a/ryu/controller/ofp_handler.py b/ryu/controller/ofp_handler.py
index 3ce91989..609617d7 100644
--- a/ryu/controller/ofp_handler.py
+++ b/ryu/controller/ofp_handler.py
@@ -14,14 +14,13 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+import itertools
 import logging
 
 import ryu.base.app_manager
 
 from ryu import utils
-from ryu.controller import handler
 from ryu.controller import ofp_event
-from ryu.controller.handler import set_ev_cls, set_ev_handler
 from ryu.controller.handler import HANDSHAKE_DISPATCHER, CONFIG_DISPATCHER,\
     MAIN_DISPATCHER
 
@@ -47,7 +46,7 @@ class OFPHandler(ryu.base.app_manager.RyuApp):
         self.name = 'ofp_event'
 
     @staticmethod
-    def hello_failed(datapath, error_desc):
+    def _hello_failed(datapath, error_desc):
         LOG.error(error_desc)
         error_msg = datapath.ofproto_parser.OFPErrorMsg(datapath)
         error_msg.type = datapath.ofproto.OFPET_HELLO_FAILED
@@ -65,13 +64,64 @@ class OFPHandler(ryu.base.app_manager.RyuApp):
         # pre 1.0 is not supported
         elements = getattr(msg, 'elements', None)
         if elements:
-            usable_versions = []
-            for elem in elements:
-                usable_versions += elem.versions or []
+            switch_versions = set()
+            for version in itertools.chain.from_iterable(
+                    element.versions for element in elements):
+                switch_versions.add(version)
+            usable_versions = switch_versions & set(
+                datapath.supported_ofp_version)
+
+            # We didn't send our supported versions for interoperability as
+            # most switches would not understand elements at the moment.
+            # So the switch would think that the negotiated version would
+            # be max(negotiated_versions), but actual usable version is
+            # max(usable_versions).
+            negotiated_versions = set(
+                version for version in switch_versions
+                if version <= max(datapath.supported_ofp_version))
+            if negotiated_versions and not usable_versions:
+                # e.g.
+                # versions of OF 1.0 and 1.1 from switch
+                # max of OF 1.2 from Ryu and supported_ofp_version = (1.2, )
+                # negotiated version = 1.1
+                # usable version = None
+                error_desc = (
+                    'no compatible version found: '
+                    'switch versions %s controller version 0x%x, '
+                    'the negotiated version is 0x%x, '
+                    'but no usable version found. '
+                    'If possible, set the switch to use one of OF version %s'
+                    % (switch_versions, max(datapath.supported_ofp_version),
+                       max(negotiated_versions),
+                       sorted(datapath.supported_ofp_version)))
+                self._hello_failed(datapath, error_desc)
+                return
+            if (negotiated_versions and usable_versions and
+                    max(negotiated_versions) != max(usable_versions)):
+                # e.g.
+                # versions of OF 1.0 and 1.1 from switch
+                # max of OF 1.2 from Ryu and supported_ofp_version = (1.0, 1.2)
+                # negotiated version = 1.1
+                # usable version = 1.0
+                #
+                # TODO: In order to get the version 1.0, Ryu need to send
+                # supported verions.
+                error_desc = (
+                    'no compatible version found: '
+                    'switch versions 0x%x controller version 0x%x, '
+                    'the negotiated version is %s but found usable %s. '
+                    'If possible, '
+                    'set the switch to use one of OF version %s' % (
+                        max(switch_versions),
+                        max(datapath.supported_ofp_version),
+                        sorted(negotiated_versions),
+                        sorted(usable_versions), sorted(usable_versions)))
+                self._hello_failed(datapath, error_desc)
+                return
         else:
-            usable_versions = [version for version
-                               in datapath.supported_ofp_version
-                               if version <= msg.version]
+            usable_versions = set(version for version
+                                  in datapath.supported_ofp_version
+                                  if version <= msg.version)
             if (usable_versions and
                 max(usable_versions) != min(msg.version,
                                             datapath.ofproto.OFP_VERSION)):
@@ -102,19 +152,21 @@ class OFPHandler(ryu.base.app_manager.RyuApp):
                 # and optionally an ASCII string explaining the situation
                 # in data, and then terminate the connection.
                 version = max(usable_versions)
-                error_desc = 'no compatible version found: '
-                'switch 0x%x controller 0x%x, but found usable 0x%x. '
-                'If possible, set the switch to use OF version 0x%x' % (
-                    msg.version, datapath.ofproto.OFP_VERSION,
-                    version, version)
-                self.hello_failed(error_desc)
+                error_desc = (
+                    'no compatible version found: '
+                    'switch 0x%x controller 0x%x, but found usable 0x%x. '
+                    'If possible, set the switch to use OF version 0x%x' % (
+                        msg.version, datapath.ofproto.OFP_VERSION,
+                        version, version))
+                self._hello_failed(datapath, error_desc)
                 return
 
         if not usable_versions:
-            error_desc = 'unsupported version 0x%x. '
-            'If possible, set the switch to use one of the versions %s' % (
-                msg.version, datapath.supported_ofp_version.keys())
-            self.hello_failed(error_desc)
+            error_desc = (
+                'unsupported version 0x%x. '
+                'If possible, set the switch to use one of the versions %s' % (
+                    msg.version, sorted(datapath.supported_ofp_version)))
+            self._hello_failed(datapath, error_desc)
             return
         datapath.set_version(max(usable_versions))