ofproto: Avoid emitting illegal instruction sets

The OpenFlow spec forbids multiple occurences of a same instruction
type within a mod_flow message, so make sure
ofp_instruction_from_jsondict doesn't emit such an instruction set.

Signed-off-by: IWAMOTO Toshihiro <iwamoto@valinux.co.jp>
Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>

2 files changed, 29 insertions, 15 deletions

diff --git a/ryu/ofproto/ofproto_parser.py b/ryu/ofproto/ofproto_parser.py
index 7b56968d..e2300558 100644
--- a/ryu/ofproto/ofproto_parser.py
+++ b/ryu/ofproto/ofproto_parser.py
@@ -147,12 +147,17 @@ def ofp_instruction_from_jsondict(dp, jsonlist, encap=True):
     """
     proto = dp.ofproto
     parser = dp.ofproto_parser
+    actions = []
     result = []
     for jsondict in jsonlist:
         assert len(jsondict) == 1
         k, v = list(jsondict.items())[0]
         cls = getattr(parser, k)
-        if not issubclass(cls, parser.OFPAction):
+        if issubclass(cls, parser.OFPAction):
+            if encap:
+                actions.append(cls.from_jsondict(v))
+                continue
+        else:
             ofpinst = getattr(parser, 'OFPInstruction', None)
             if not ofpinst or not issubclass(cls, ofpinst):
                 raise ValueError("Supplied jsondict is of wrong type: %s",
@@ -161,20 +166,13 @@ def ofp_instruction_from_jsondict(dp, jsonlist, encap=True):
 
     if not encap:
         return result
-    insts = []
-    actions = []
-    result.append(None)  # sentinel
-    for act_or_inst in result:
-        if isinstance(act_or_inst, parser.OFPAction):
-            actions.append(act_or_inst)
-        else:
-            if actions:
-                insts.append(parser.OFPInstructionActions(
-                    proto.OFPIT_APPLY_ACTIONS, actions))
-                actions = []
-            if act_or_inst is not None:
-                insts.append(act_or_inst)
-    return insts
+
+    if actions:
+        # Although the OpenFlow spec says Apply Actions is executed first,
+        # let's place it in the head as a precaution.
+        result = [parser.OFPInstructionActions(
+            proto.OFPIT_APPLY_ACTIONS, actions)] + result
+    return result
 
 
 class StringifyMixin(stringify.StringifyMixin):
diff --git a/ryu/tests/unit/lib/test_ofctl_string.py b/ryu/tests/unit/lib/test_ofctl_string.py
index fb491e39..cc0ce9f2 100644
--- a/ryu/tests/unit/lib/test_ofctl_string.py
+++ b/ryu/tests/unit/lib/test_ofctl_string.py
@@ -149,3 +149,19 @@ class Test_OfctlString(unittest.TestCase):
                         {'len': 8,
                          'table_id': 33,
                          'type': 1}})
+
+    def test_multi_unordered(self):
+        self._test_str(self.fake_dp_of15,
+                       'pop_vlan,goto_table:33,output:1',
+                       {'OFPInstructionActions':
+                        {'actions': [{'OFPActionPopVlan': {'len': 8,
+                                                           'type': 18}},
+                                     {'OFPActionOutput': {'len': 16,
+                                                          'max_len': 65509,
+                                                          'port': 1,
+                                                          'type': 0}}],
+                         'type': 4}},
+                       {'OFPInstructionGotoTable':
+                        {'len': 8,
+                         'table_id': 33,
+                         'type': 1}})