From edd70aa7715800f4b109f879e448c8efa40dadd5 Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Fri, 30 Nov 2012 14:18:03 -0800 Subject: [PATCH] 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 --- lib/ofp-errors.c | 14 ++------------ lib/ofp-errors.h | 1 - tests/ofp-errors.at | 21 +++++++++++++++++++++ utilities/ovs-ofctl.c | 35 +++++++++++++++++++++++++++++++++++ 4 files changed, 58 insertions(+), 13 deletions(-) diff --git a/lib/ofp-errors.c b/lib/ofp-errors.c index f2df8aec0..697e5a609 100644 --- 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 5c3df20d9..9f6911ca8 100644 --- 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 9ed33e502..e99aca9af 100644 --- 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 df2c6aa06..26736bb9b 100644 --- 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 }, -- 2.47.0