[ovs-dev] [ofp-errors 5/5] ofp-errors: Make every error encodable.

Ben Pfaff blp at nicira.com
Mon Dec 3 19:20:33 UTC 2012


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 <jarno.rajahalme at nsn.com>
Signed-off-by: Ben Pfaff <blp at nicira.com>
---
 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 5997334..6b3a42c 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 9d52954..593241d 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 3851a8e..9acd90e 100644
--- a/ofproto/connmgr.c
+++ b/ofproto/connmgr.c
@@ -953,29 +953,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'. */
-- 
1.7.2.5




More information about the dev mailing list