ofp-errors: Correctly encode errors as extensions or not depending on domain.

When ofp-errors was introduced, each OFPERR_* was either an extension or
not.  However, since then, some Nicira extension error code have been
given official error codes in later OpenFlow versions, so now whether an
OFPERR_* is an extension depends on the OpenFlow versions.  This means
that certain errors were encoded incorrectly as extensions in later
OpenFlow versions.  This commit fixes the problem.

This commit also adds a test that should prevent a regression.

Signed-off-by: Ben Pfaff <blp@nicira.com>

diff --git a/lib/ofp-errors.c b/lib/ofp-errors.c
index f2df8ae..697e5a6 100644 (file)
--- a/lib/ofp-errors.c
+++ b/lib/ofp-errors.c
@@ -63,17 +63,6 @@ ofperr_is_category(enum ofperr error)
             && ofperr_of10.errors[error - OFPERR_OFS].code == -1
             && ofperr_of11.errors[error - OFPERR_OFS].code == -1);
 }
-
-/* Returns true if 'error' is a valid OFPERR_* value that is a Nicira
- * extension, e.g. if it is an OFPERR_NX* value, and false otherwise. */
-bool
-ofperr_is_nx_extension(enum ofperr error)
-{
-    return (ofperr_is_valid(error)
-            && (ofperr_of10.errors[error - OFPERR_OFS].code >= 0x100 ||
-                ofperr_of11.errors[error - OFPERR_OFS].code >= 0x100));
-}
-
 /* Returns true if 'error' can be encoded as an OpenFlow error message in
  * 'domain', false otherwise.
  *
@@ -192,7 +181,7 @@ ofperr_encode_msg__(enum ofperr error, enum ofp_version ofp_version,
     }
 
     pair = ofperr_get_pair__(error, domain);
-    if (!ofperr_is_nx_extension(error)) {
+    if (pair->code < 0x100) {
         buf = ofpraw_alloc_xid(OFPRAW_OFPT_ERROR, domain->version, xid,
                                sizeof *oem + data_len);
 
@@ -216,6 +205,7 @@ ofperr_encode_msg__(enum ofperr error, enum ofp_version ofp_version,
     }
 
     ofpbuf_put(buf, data, data_len);
+    ofpmsg_update_length(buf);
 
     return buf;
 }

diff --git a/lib/ofp-errors.h b/lib/ofp-errors.h
index 5c3df20..9f6911c 100644 (file)
--- a/lib/ofp-errors.h
+++ b/lib/ofp-errors.h
@@ -583,7 +583,6 @@ const char *ofperr_domain_get_name(enum ofp_version);
 
 bool ofperr_is_valid(enum ofperr);
 bool ofperr_is_category(enum ofperr);
-bool ofperr_is_nx_extension(enum ofperr);
 bool ofperr_is_encodable(enum ofperr, enum ofp_version);
 
 enum ofperr ofperr_decode(enum ofp_version, uint16_t type, uint16_t code);

diff --git a/tests/ofp-errors.at b/tests/ofp-errors.at
index 9ed33e5..e99aca9 100644 (file)
--- a/tests/ofp-errors.at
+++ b/tests/ofp-errors.at
@@ -100,6 +100,27 @@ OpenFlow 1.3: 3,6
 ])
 AT_CLEANUP
 
+dnl The "bad role" error was a Nicira extension in OpenFlow 1.0 and 1.1.
+dnl It was adopted as an official error code in OpenFlow 1.2.
+AT_SETUP([encoding errors extension that became official])
+AT_KEYWORDS([ofp-print ofp-errors])
+AT_CHECK(
+  [ovs-ofctl encode-error-reply OFPRRFC_BAD_ROLE 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 01 01 00 00 08-12 34 56 78 @&t@
+])
+AT_CHECK(
+  [ovs-ofctl encode-error-reply OFPRRFC_BAD_ROLE 0200000812345678], [0], [dnl
+00000000  02 01 00 1c 12 34 56 78-b0 c2 00 00 00 00 23 20 @&t@
+00000010  00 01 02 01 02 00 00 08-12 34 56 78 @&t@
+])
+AT_CHECK(
+  [ovs-ofctl encode-error-reply OFPRRFC_BAD_ROLE 0300000812345678], [0], [dnl
+00000000  03 01 00 14 12 34 56 78-00 0b 00 02 03 00 00 08 @&t@
+00000010  12 34 56 78 @&t@
+])
+AT_CLEANUP
+
 AT_SETUP([decoding OFPBIC_* experimenter errors])
 AT_KEYWORDS([ofp-print ofp-errors])
 AT_CHECK([ovs-ofctl ofp-print '0201001455555555 00030005 0102000811111111'], [0], [dnl

diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
index df2c6aa..26736bb 100644 (file)
--- a/utilities/ovs-ofctl.c
+++ b/utilities/ovs-ofctl.c
@@ -2751,6 +2751,40 @@ ofctl_print_error(int argc OVS_UNUSED, char *argv[])
     }
 }
 
+/* "encode-error-reply ENUM REQUEST": Encodes an error reply to REQUEST for the
+ * error named ENUM and prints the error reply in hex. */
+static void
+ofctl_encode_error_reply(int argc OVS_UNUSED, char *argv[])
+{
+    const struct ofp_header *oh;
+    struct ofpbuf request, *reply;
+    enum ofperr error;
+
+    error = ofperr_from_name(argv[1]);
+    if (!error) {
+        ovs_fatal(0, "unknown error \"%s\"", argv[1]);
+    }
+
+    ofpbuf_init(&request, 0);
+    if (ofpbuf_put_hex(&request, argv[2], NULL)[0] != '\0') {
+        ovs_fatal(0, "Trailing garbage in hex data");
+    }
+    if (request.size < sizeof(struct ofp_header)) {
+        ovs_fatal(0, "Request too short");
+    }
+
+    oh = request.data;
+    if (request.size != ntohs(oh->length)) {
+        ovs_fatal(0, "Request size inconsistent");
+    }
+
+    reply = ofperr_encode_reply(error, request.data);
+    ofpbuf_uninit(&request);
+
+    ovs_hex_dump(stdout, reply->data, reply->size, 0, false);
+    ofpbuf_delete(reply);
+}
+
 /* "ofp-print HEXSTRING [VERBOSITY]": Converts the hex digits in HEXSTRING into
  * binary data, interpreting them as an OpenFlow message, and prints the
  * OpenFlow message on stdout, at VERBOSITY (level 2 by default).  */
@@ -2820,6 +2854,7 @@ static const struct command all_commands[] = {
     { "parse-ofp11-instructions", 0, 0, ofctl_parse_ofp11_instructions },
     { "check-vlan", 2, 2, ofctl_check_vlan },
     { "print-error", 1, 1, ofctl_print_error },
+    { "encode-error-reply", 2, 2, ofctl_encode_error_reply },
     { "ofp-print", 1, 2, ofctl_ofp_print },
     { "encode-hello", 1, 1, ofctl_encode_hello },