ofp-errors: Fix bugs in treatment of OpenFlow experimenter errors.

OpenFlow 1.2 and later have "experimenter errors".  The OVS implementation
was buggy in a few ways.  First, a bug in extract-ofp-errors prevented
OF1.2+ experimenter errors from being properly decoded.  Second,
OF1.2+ experimenter errors have only a type, not a code, whereas all other
types of errors (standard errors, OF1.0/1.1 Nicira extension errors) have
both, but extract-ofp-errors didn't properly enforce that.

This commit fixes both problems and improves existing tests to verify that
encoding and decoding of experimenter errors now works properly.

This commit also fixes the definition of OFPBIC_DUP_INST.  It claimed to
have an OF1.1 experimenter error value although OF1.1 didn't have
experimenter errors.  This commit changes it to use a Nicira extension
error in OF1.1 instead.

Signed-off-by: Ben Pfaff <blp@nicira.com>
Acked-by: Jarno Rajahalme <jrajahalme@nicira.com>

3 files changed, 72 insertions, 7 deletions

diff --git a/build-aux/extract-ofp-errors b/build-aux/extract-ofp-errors
index 89dd31a3d..16bfbc721 100755
--- a/build-aux/extract-ofp-errors
+++ b/build-aux/extract-ofp-errors
@@ -300,14 +300,34 @@ def extract_ofp_errors(fn):
                       % (version1_name, version2_name,
                          version2_name, version1_name))
 
-            if vendor == vendor_map['NX']:
+            if vendor == vendor_map['OF']:
+                # All standard OpenFlow errors have a type and a code.
+                if code is None:
+                    fatal("%s: %s domain requires code" % (dst, vendor_name))
+            elif vendor == vendor_map['NX']:
+                # Before OpenFlow 1.2, OVS used a Nicira extension to
+                # define errors that included a type and a code.
+                #
+                # In OpenFlow 1.2 and later, Nicira extension errors
+                # are defined using the OpenFlow experimenter error
+                # mechanism that includes a type but not a code.
+                if v1 < version_map['1.2'] or v2 < version_map['1.2']:
+                    if code is None:
+                        fatal("%s: NX1.0 and NX1.1 domains require code"
+                              % (dst, vendor_name))
                 if v1 >= version_map['1.2'] or v2 >= version_map['1.2']:
                     if code is not None:
                         fatal("%s: NX1.2+ domains do not have codes" % dst)
-                    code = 0
-            elif vendor != vendor_map['OF']:
+            else:
+                # Experimenter extension error for OF1.2+ only.
+                if v1 < version_map['1.2']:
+                    fatal("%s: %s domain not supported before OF1.2"
+                          % (dst, vendor_name))
                 if code is not None:
-                    fatal("%s: %s domains do not have codes" % vendor_name)
+                    fatal("%s: %s domains do not have codes"
+                          % (dst, vendor_name))
+            if code is None:
+                code = 0
 
             for version in range(v1, v2 + 1):
                 domain[version].setdefault(vendor, {})
@@ -376,8 +396,6 @@ static enum ofperr
             if enum not in map:
                 continue
             vendor, type_, code = map[enum]
-            if code is None:
-                continue
             value = (vendor << 32) | (type_ << 16) | code
             if value in found:
                 continue
diff --git a/lib/ofp-errors.h b/lib/ofp-errors.h
index 643fa72b0..0d5bb3a10 100644
--- a/lib/ofp-errors.h
+++ b/lib/ofp-errors.h
@@ -262,7 +262,7 @@ enum ofperr {
     /* OF1.2+(3,8).  Permissions error. */
     OFPERR_OFPBIC_EPERM,
 
-    /* ONF1.1-1.3(2600), OF1.4+(3,9).  Duplicate instruction. */
+    /* NX1.1(3,256), ONF1.2-1.3(2600), OF1.4+(3,9).  Duplicate instruction. */
     OFPERR_OFPBIC_DUP_INST,
 
 /* ## --------------- ## */
diff --git a/tests/ofp-errors.at b/tests/ofp-errors.at
index f8c5cc482..daf7ddd24 100644
--- a/tests/ofp-errors.at
+++ b/tests/ofp-errors.at
@@ -163,10 +163,29 @@ AT_CHECK([ovs-ofctl ofp-print '0301001855555555 ffff0004 00002320 01020008111111
 OFPT_ERROR (OF1.2) (xid=0x55555555): NXBRC_MUST_BE_ZERO
 OFPT_ECHO_REQUEST (xid=0x11111111): 0 bytes of payload
 ])
+
+AT_CHECK([ovs-ofctl ofp-print '0301001812345678 ffff0a28 4f4e4600 0300000812345678'], [0], [dnl
+OFPT_ERROR (OF1.2) (xid=0x12345678): OFPBIC_DUP_INST
+OFPT_HELLO (OF1.2) (xid=0x12345678):
+ version bitmap: 0x01, 0x02, 0x03
+])
+AT_CHECK([ovs-ofctl ofp-print '0401001812345678 ffff0a28 4f4e4600 0400000812345678'], [0], [dnl
+OFPT_ERROR (OF1.3) (xid=0x12345678): OFPBIC_DUP_INST
+OFPT_HELLO (OF1.3) (xid=0x12345678):
+ version bitmap: 0x01, 0x02, 0x03, 0x04
+])
+AT_CHECK([ovs-ofctl ofp-print '0501001412345678 00030009 0500000812345678'], [0], [dnl
+OFPT_ERROR (OF1.4) (xid=0x12345678): OFPBIC_DUP_INST
+OFPT_HELLO (OF1.4) (xid=0x12345678):
+ version bitmap: 0x01, 0x02, 0x03, 0x04, 0x05
+])
 AT_CLEANUP
 
 AT_SETUP([encoding experimenter errors])
 AT_KEYWORDS([ofp-print ofp-errors])
+# Demonstrate that a Nicira extension error gets encoded correctly
+# using the Nicira error extension format in OF1.0 and OF1.1, and
+# correctly using the standard experimenter format in OF1.2.
 AT_CHECK(
   [ovs-ofctl encode-error-reply NXBRC_MUST_BE_ZERO 0100000812345678], [0], [dnl
 00000000  01 01 00 1c 12 34 56 78-b0 c2 00 00 00 00 23 20 @&t@
@@ -182,4 +201,32 @@ AT_CHECK(
 00000000  03 01 00 18 12 34 56 78-ff ff 00 04 00 00 23 20 @&t@
 00000010  03 00 00 08 12 34 56 78-
 ])
+
+# Check that OFPERR_OFPBIC_DUP_INST is:
+#    - not encodable in OF1.0 (OF1.0 doesn't have instructions, after all).
+#    - encoded as a Nicira extension in OF1.1.
+#    - encoded as an ONF extension in OF1.2 and OF1.3.
+#    - encoded in the standard form in OF1.4.
+AT_CHECK(
+  [ovs-ofctl '-vPATTERN:console:%c|%p|%m' encode-error-reply OFPBIC_DUP_INST 0100000812345678], [0], [dnl
+00000000  01 01 00 1c 12 34 56 78-b0 c2 00 00 00 00 23 20 @&t@
+00000010  00 01 02 09 01 00 00 08-12 34 56 78 @&t@
+], [ofp_errors|ERR|cannot encode OFPBIC_DUP_INST for OpenFlow 1.0
+])
+AT_CHECK([ovs-ofctl encode-error-reply OFPBIC_DUP_INST 0200000812345678], [0],
+[00000000  02 01 00 1c 12 34 56 78-b0 c2 00 00 00 00 23 20 @&t@
+00000010  00 03 01 00 02 00 00 08-12 34 56 78 @&t@
+])
+AT_CHECK([ovs-ofctl encode-error-reply OFPBIC_DUP_INST 0300000812345678], [0],
+[00000000  03 01 00 18 12 34 56 78-ff ff 0a 28 4f 4e 46 00 @&t@
+00000010  03 00 00 08 12 34 56 78-
+])
+AT_CHECK([ovs-ofctl encode-error-reply OFPBIC_DUP_INST 0400000812345678], [0],
+[00000000  04 01 00 18 12 34 56 78-ff ff 0a 28 4f 4e 46 00 @&t@
+00000010  04 00 00 08 12 34 56 78-
+])
+AT_CHECK([ovs-ofctl encode-error-reply OFPBIC_DUP_INST 0500000812345678], [0],
+[00000000  05 01 00 14 12 34 56 78-00 03 00 09 05 00 00 08 @&t@
+00000010  12 34 56 78 @&t@
+])
 AT_CLEANUP