From 07c8ec217b5641b74f20b216ad63fef230649f2c Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Fri, 30 Nov 2012 16:22:52 -0800 Subject: [PATCH] ofp-errors: Make every error encodable. Until now, some values could not be encoded to send over OpenFlow, for a few possible reasons. This meant that, when one of these situations came up, that a controller would not receive any notification that its request failed. This is not a good way to behave, so this commit changes the error encoder so that, if a particular error cannot be encoded, it will instead encode a fallback error code. This commit also slightly simplifies ofconn_send_error() because it no longer has to handle ofperr_encode_error() returning NULL. Reported-by: Jarno Rajahalme Signed-off-by: Ben Pfaff --- lib/ofp-errors.c | 67 ++++++++++++----------------------------------- lib/ofp-errors.h | 5 +++- ofproto/connmgr.c | 37 ++++++++++++-------------- 3 files changed, 38 insertions(+), 71 deletions(-) diff --git a/lib/ofp-errors.c b/lib/ofp-errors.c index 599733415..6b3a42cf9 100644 --- a/lib/ofp-errors.c +++ b/lib/ofp-errors.c @@ -53,19 +53,6 @@ ofperr_is_valid(enum ofperr error) return error >= OFPERR_OFS && error < OFPERR_OFS + OFPERR_N_ERRORS; } -/* Returns true if 'error' can be encoded as an OpenFlow error message in - * 'domain', false otherwise. - * - * A given error may not be encodable in some domains because each OpenFlow - * version tends to introduce new errors and retire some old ones. */ -bool -ofperr_is_encodable(enum ofperr error, enum ofp_version version) -{ - const struct ofperr_domain *domain = ofperr_domain_from_version(version); - return (ofperr_is_valid(error) - && domain && domain->errors[error - OFPERR_OFS].code >= 0); -} - /* Returns the OFPERR_* value that corresponds to 'type' and 'code' within * 'version', or 0 if either no such OFPERR_* value exists or 'version' is * unknown. */ @@ -131,33 +118,30 @@ static struct ofpbuf * ofperr_encode_msg__(enum ofperr error, enum ofp_version ofp_version, ovs_be32 xid, const void *data, size_t data_len) { + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); + const struct ofperr_domain *domain; struct ofp_error_msg *oem; const struct pair *pair; struct ofpbuf *buf; - const struct ofperr_domain *domain; + /* Get the error domain for 'ofp_version', or fall back to OF1.0. */ domain = ofperr_domain_from_version(ofp_version); if (!domain) { - return NULL; + VLOG_ERR_RL(&rl, "cannot encode error for unknown OpenFlow " + "version 0x%02x", ofp_version); + domain = &ofperr_of10; } - if (!ofperr_is_encodable(error, ofp_version)) { - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); - - if (!ofperr_is_valid(error)) { - /* 'error' seems likely to be a system errno value. */ - VLOG_WARN_RL(&rl, "invalid OpenFlow error code %d (%s)", - error, strerror(error)); - } else { - const char *s = ofperr_get_name(error); - if (ofperr_is_category(error)) { - VLOG_WARN_RL(&rl, "cannot encode error category (%s)", s); - } else { - VLOG_WARN_RL(&rl, "cannot encode %s for %s", s, domain->name); - } - } - - return NULL; + /* Make sure 'error' is valid in 'domain', or use a fallback error. */ + if (!ofperr_is_valid(error)) { + /* 'error' seems likely to be a system errno value. */ + VLOG_ERR_RL(&rl, "invalid OpenFlow error code %d (%s)", + error, strerror(error)); + error = OFPERR_NXBRC_UNENCODABLE_ERROR; + } else if (domain->errors[error - OFPERR_OFS].code < 0) { + VLOG_ERR_RL(&rl, "cannot encode %s for %s", + ofperr_get_name(error), domain->name); + error = OFPERR_NXBRC_UNENCODABLE_ERROR; } pair = ofperr_get_pair__(error, domain); @@ -198,9 +182,6 @@ ofperr_encode_msg__(enum ofperr error, enum ofp_version ofp_version, * The error reply will contain an initial subsequence of 'oh', up to * 'oh->length' or 64 bytes, whichever is shorter. * - * Returns NULL if 'error' is not an OpenFlow error code or if 'error' cannot - * be encoded as OpenFlow version 'oh->version'. - * * This function isn't appropriate for encoding OFPET_HELLO_FAILED error * messages. Use ofperr_encode_hello() instead. */ struct ofpbuf * @@ -217,25 +198,11 @@ ofperr_encode_reply(enum ofperr error, const struct ofp_header *oh) * * If 'version' is an unknown version then OFP10_VERSION is used. * OFPET_HELLO_FAILED error messages are supposed to be backward-compatible, - * so in theory this should work. - * - * Returns NULL if 'error' is not an OpenFlow error code or if 'error' cannot - * be encoded in 'domain'. */ + * so in theory this should work. */ struct ofpbuf * ofperr_encode_hello(enum ofperr error, enum ofp_version ofp_version, const char *s) { - switch (ofp_version) { - case OFP10_VERSION: - case OFP11_VERSION: - case OFP12_VERSION: - case OFP13_VERSION: - break; - - default: - ofp_version = OFP10_VERSION; - } - return ofperr_encode_msg__(error, ofp_version, htonl(0), s, strlen(s)); } diff --git a/lib/ofp-errors.h b/lib/ofp-errors.h index 9d529549f..593241dec 100644 --- a/lib/ofp-errors.h +++ b/lib/ofp-errors.h @@ -160,6 +160,10 @@ enum ofperr { * NXFME_MODIFY. */ OFPERR_NXBRC_FM_BAD_EVENT, + /* NX1.0+(1,521). The error that occurred cannot be represented in this + * OpenFlow version. */ + OFPERR_NXBRC_UNENCODABLE_ERROR, + /* ## ---------------- ## */ /* ## OFPET_BAD_ACTION ## */ /* ## ---------------- ## */ @@ -537,7 +541,6 @@ enum ofperr { const char *ofperr_domain_get_name(enum ofp_version); bool ofperr_is_valid(enum ofperr); -bool ofperr_is_encodable(enum ofperr, enum ofp_version); enum ofperr ofperr_decode(enum ofp_version, uint16_t type, uint16_t code); enum ofperr ofperr_from_name(const char *); diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c index 928c56d03..77b7b3916 100644 --- a/ofproto/connmgr.c +++ b/ofproto/connmgr.c @@ -976,29 +976,26 @@ void ofconn_send_error(const struct ofconn *ofconn, const struct ofp_header *request, enum ofperr error) { + static struct vlog_rate_limit err_rl = VLOG_RATE_LIMIT_INIT(10, 10); struct ofpbuf *reply; reply = ofperr_encode_reply(error, request); - if (reply) { - static struct vlog_rate_limit err_rl = VLOG_RATE_LIMIT_INIT(10, 10); - - if (!VLOG_DROP_INFO(&err_rl)) { - const char *type_name; - size_t request_len; - enum ofpraw raw; - - request_len = ntohs(request->length); - type_name = (!ofpraw_decode_partial(&raw, request, - MIN(64, request_len)) - ? ofpraw_get_name(raw) - : "invalid"); - - VLOG_INFO("%s: sending %s error reply to %s message", - rconn_get_name(ofconn->rconn), ofperr_to_string(error), - type_name); - } - ofconn_send_reply(ofconn, reply); - } + if (!VLOG_DROP_INFO(&err_rl)) { + const char *type_name; + size_t request_len; + enum ofpraw raw; + + request_len = ntohs(request->length); + type_name = (!ofpraw_decode_partial(&raw, request, + MIN(64, request_len)) + ? ofpraw_get_name(raw) + : "invalid"); + + VLOG_INFO("%s: sending %s error reply to %s message", + rconn_get_name(ofconn->rconn), ofperr_to_string(error), + type_name); + } + ofconn_send_reply(ofconn, reply); } /* Same as pktbuf_retrieve(), using the pktbuf owned by 'ofconn'. */ -- 2.47.0