ofp-util: Add infrastructure for vendor extensions to OpenFlow error codes.

Cross-ported from "wdp" branch.

include/openflow/nicira-ext.h

@@ -18,14 +18,58 @@
 #define OPENFLOW_NICIRA_EXT_H 1
 
 #include "openflow/openflow.h"
+#include "openvswitch/types.h"
 
 #define NICIRA_OUI_STR "002320"
 
 /* The following vendor extensions, proposed by Nicira Networks, are not yet
- * ready for standardization (and may never be), so they are not included in
+ * standardized, so they are not included in openflow.h.  Some of them may be
- * openflow.h. */
+ * suitable for standardization; others we never expect to standardize. */
 
 #define NX_VENDOR_ID 0x00002320
+
+/* Nicira vendor-specific error messages extension.
+ *
+ * OpenFlow 1.0 has a set of predefined error types (OFPET_*) and codes (which
+ * are specific to each type).  It does not have any provision for
+ * vendor-specific error codes, and it does not even provide "generic" error
+ * codes that can apply to problems not anticipated by the OpenFlow
+ * specification authors.
+ *
+ * This extension attempts to address the problem by adding a generic "error
+ * vendor extension".  The extension works as follows: use NXET_VENDOR as type
+ * and NXVC_VENDOR_CODE as code, followed by struct nx_vendor_error with
+ * vendor-specific details, followed by at least 64 bytes of the failed
+ * request.
+ *
+ * It would be better to have type-specific vendor extension, e.g. so that
+ * OFPET_BAD_ACTION could be used with vendor-specific code values.  But
+ * OFPET_BAD_ACTION and most other standardized types already specify that
+ * their 'data' values are (the start of) the OpenFlow message being replied
+ * to, so there is no room to insert a vendor ID.
+ *
+ * Currently this extension is only implemented by Open vSwitch, but it seems
+ * like a reasonable candidate for future standardization.
+ */
+
+/* This is a random number to avoid accidental collision with any other
+ * vendor's extension. */
+#define NXET_VENDOR 0xb0c2
+
+/* ofp_error msg 'code' values for NXET_VENDOR. */
+enum nx_vendor_code {
+    NXVC_VENDOR_ERROR           /* 'data' contains struct nx_vendor_error. */
+};
+
+/* 'data' for 'type' == NXET_VENDOR, 'code' == NXVC_VENDOR_ERROR. */
+struct nx_vendor_error {
+    ovs_be32 vendor;            /* Vendor ID as in struct ofp_vendor_header. */
+    ovs_be16 type;              /* Vendor-defined type. */
+    ovs_be16 code;              /* Vendor-defined subtype. */
+    /* Followed by at least the first 64 bytes of the failed request. */
+};
+
+/* Nicira vendor requests and replies. */
 
 enum nicira_type {
     /* Switch status request.  The request body is an ASCII string that
@@ -101,6 +145,8 @@ enum nx_role {
     NX_ROLE_MASTER,             /* Full access, at most one. */
     NX_ROLE_SLAVE               /* Read-only access. */
 };
+
+/* Nicira vendor flow actions. */
 
 enum nx_action_subtype {
     NXAST_SNAT__OBSOLETE,           /* No longer used. */

lib/ofp-util.c

@@ -823,3 +823,93 @@ ofp_match_to_literal_string(const struct ofp_match *match)
                      ntohs(match->tp_src),
                      ntohs(match->tp_dst));
 }
+
+static uint32_t
+vendor_code_to_id(uint8_t code)
+{
+    switch (code) {
+#define OFPUTIL_VENDOR(NAME, VENDOR_ID) case NAME: return VENDOR_ID;
+    default:
+        return UINT32_MAX;
+    }
+}
+
+/* Creates and returns an OpenFlow message of type OFPT_ERROR with the error
+ * information taken from 'error', whose encoding must be as described in the
+ * large comment in ofp-util.h.  If 'oh' is nonnull, then the error will use
+ * oh->xid as its transaction ID, and it will include up to the first 64 bytes
+ * of 'oh'.
+ *
+ * Returns NULL if 'error' is not an OpenFlow error code. */
+struct ofpbuf *
+make_ofp_error_msg(int error, const struct ofp_header *oh)
+{
+    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
+
+    struct ofpbuf *buf;
+    const void *data;
+    size_t len;
+    uint8_t vendor;
+    uint16_t type;
+    uint16_t code;
+    uint32_t xid;
+
+    if (!is_ofp_error(error)) {
+        /* We format 'error' with strerror() here since it seems likely to be
+         * a system errno value. */
+        VLOG_WARN_RL(&rl, "invalid OpenFlow error code %d (%s)",
+                     error, strerror(error));
+        return NULL;
+    }
+
+    if (oh) {
+        xid = oh->xid;
+        data = oh;
+        len = ntohs(oh->length);
+        if (len > 64) {
+            len = 64;
+        }
+    } else {
+        xid = 0;
+        data = NULL;
+        len = 0;
+    }
+
+    vendor = get_ofp_err_vendor(error);
+    type = get_ofp_err_type(error);
+    code = get_ofp_err_code(error);
+    if (vendor == OFPUTIL_VENDOR_OPENFLOW) {
+        struct ofp_error_msg *oem;
+
+        oem = make_openflow_xid(len + sizeof *oem, OFPT_ERROR, xid, &buf);
+        oem->type = htons(type);
+        oem->code = htons(code);
+    } else {
+        struct ofp_error_msg *oem;
+        struct nx_vendor_error *ove;
+        uint32_t vendor_id;
+
+        vendor_id = vendor_code_to_id(vendor);
+        if (vendor_id == UINT32_MAX) {
+            VLOG_WARN_RL(&rl, "error %x contains invalid vendor code %d",
+                         error, vendor);
+            return NULL;
+        }
+
+        oem = make_openflow_xid(len + sizeof *oem + sizeof *ove,
+                                OFPT_ERROR, xid, &buf);
+        oem->type = htons(NXET_VENDOR);
+        oem->code = htons(NXVC_VENDOR_ERROR);
+
+        ove = ofpbuf_put_uninit(buf, sizeof *ove);
+        ove->vendor = htonl(vendor_id);
+        ove->type = htons(type);
+        ove->code = htons(code);
+    }
+
+    if (len) {
+        ofpbuf_put(buf, data, len);
+    }
+
+    return buf;
+}

lib/ofp-util.h

@@ -83,57 +83,111 @@ bool action_outputs_to_port(const union ofp_action *, uint16_t port);
 void normalize_match(struct ofp_match *);
 char *ofp_match_to_literal_string(const struct ofp_match *match);
 
-/* OpenFlow errors.
+/* OpenFlow vendors.
  *
- * OpenFlow errors have two 16-bit parts: a "type" and a "code".  A "type" has
+ * These functions map vendor */
- * a unique meaning.  The "code" values are different for each "type".
+/* Vendor error numbers currently used in Open vSwitch. */
+#define OFPUTIL_VENDORS                                     \
+    /*             vendor name              vendor value */ \
+    OFPUTIL_VENDOR(OFPUTIL_VENDOR_OPENFLOW, 0x00000000)     \
+    OFPUTIL_VENDOR(OFPUTIL_VENDOR_NICIRA,   0x00002320)
+
+/* OFPUTIL_VENDOR_* definitions. */
+enum ofputil_vendor_codes {
+#define OFPUTIL_VENDOR(NAME, VENDOR_ID) NAME,
+    OFPUTIL_VENDORS
+    OFPUTIL_N_VENDORS
+#undef OFPUTIL_VENDOR
+};
+
+/* Error codes.
  *
- * We embed OpenFlow errors in the same space as errno values by shifting
+ * We embed system errno values and OpenFlow standard and vendor extension
- * 'type' left 16 bits and adding the 'code'.  An "int" value is thus broken
+ * error codes into a single 31-bit space using the following encoding.
- * into a few different ranges:
+ * (Bit 31 is unused and assumed 0 to avoid negative "int" values.)
  *
- *      - 0: success.
+ *   31                                                   0
+ *  +------------------------------------------------------+
+ *  |                           0                          |  success
+ *  +------------------------------------------------------+
  *
- *      - 1...65535: system errno values.
+ *   30 29                                                0
+ *  +--+---------------------------------------------------+
+ *  |0 |                    errno value                    |  errno value
+ *  +--+---------------------------------------------------+
  *
- *        The assumption that system errno values are less than 65536 is true
+ *   30 29   26 25            16 15                       0
- *        on at least Linux, FreeBSD, OpenBSD, and Windows.  RFC 1813 defines
+ *  +--+-------+----------------+--------------------------+
- *        NFSv3-specific errno codes starting at 10000, another hint that this
+ *  |1 |   0   |      type      |           code           |  standard OpenFlow
- *        is a reasonable assumption.
+ *  +--+-------+----------------+--------------------------+  error
  *
- *        C and POSIX say that errno values are positive.
+ *   30 29   26 25            16 15                       0
+ *  +--+-------+----------------+--------------------------+  Nicira
+ *  | 1| vendor|      type      |           code           |  NXET_VENDOR
+ *  +--+-------+----------------+--------------------------+  error extension
  *
- *      - 65536...INT_MAX: OpenFlow errors.
+ * C and POSIX say that errno values are positive.  We assume that they are
+ * less than 2**29.  They are actually less than 65536 on at least Linux,
+ * FreeBSD, OpenBSD, and Windows.
  *
- *        In OpenFlow, a "type" of 0 is valid, but it corresponds to
+ * The 'vendor' field holds one of the OFPUTIL_VENDOR_* codes defined above.
- *        OFPET_HELLO_FAILED.  That's not a general-purpose error: only the
+ * It must be nonzero.
- *        vconn library would ever care to send it.  So we ignore it.
  *
- *      - negative values: not used.
+ * Negative values are not defined.
  */
 
-/* Returns the OpenFlow error with the specified 'type' and 'code' as an
+/* Currently 4 bits are allocated to the "vendor" field.  Make sure that all
- * integer. */
+ * the vendor codes can fit. */
+BUILD_ASSERT_DECL(OFPUTIL_N_VENDORS <= 16);
+
+/* Returns the standard OpenFlow error with the specified 'type' and 'code' as
+ * an integer. */
 static inline int
 ofp_mkerr(uint16_t type, uint16_t code)
 {
-    assert(type > 0 && type <= 0x7fff);
+    return (1 << 30) | (type << 16) | code;
-    return (type << 16) | code;
 }
 
-/* Returns true if 'error' is in the range of values used as OpenFlow error
+/* Returns the OpenFlow vendor error with the specified 'vendor', 'type', and
- * codes as explained above. */
+ * 'code' as an integer.  'vendor' must be an OFPUTIL_VENDOR_* constant. */
+static inline int
+ofp_mkerr_vendor(uint8_t vendor, uint16_t type, uint16_t code)
+{
+    assert(vendor < OFPUTIL_N_VENDORS);
+    return (1 << 30) | (vendor << 26) | (type << 16) | code;
+}
+
+/* Returns the OpenFlow vendor error with Nicira as vendor, with the specific
+ * 'type' and 'code', as an integer. */
+static inline int
+ofp_mkerr_nicira(uint16_t type, uint16_t code)
+{
+    return ofp_mkerr_vendor(OFPUTIL_VENDOR_NICIRA, type, code);
+}
+
+/* Returns true if 'error' encodes an OpenFlow standard or vendor extension
+ * error codes as documented above. */
 static inline bool
 is_ofp_error(int error)
 {
-    return error >= 0x10000;
+    return (error & (1 << 30)) != 0;
 }
 
 /* Returns true if 'error' appears to be a system errno value. */
 static inline bool
 is_errno(int error)
 {
-    return error < 0x10000;
+    return !is_ofp_error(error);
+}
+
+/* Returns the "vendor" part of the OpenFlow error code 'error' (which must be
+ * in the format explained above).  This is normally one of the
+ * OFPUTIL_VENDOR_* constants.  Returns OFPUTIL_VENDOR_OPENFLOW (0) for a
+ * standard OpenFlow error. */
+static inline uint8_t
+get_ofp_err_vendor(int error)
+{
+    return (error >> 26) & 0xf;
 }
 
 /* Returns the "type" part of the OpenFlow error code 'error' (which must be in
@@ -141,7 +195,7 @@ is_errno(int error)
 static inline uint16_t
 get_ofp_err_type(int error)
 {
-    return error >> 16;
+    return (error >> 16) & 0x3ff;
 }
 
 /* Returns the "code" part of the OpenFlow error code 'error' (which must be in
@@ -152,4 +206,6 @@ get_ofp_err_code(int error)
     return error & 0xffff;
 }
 
+struct ofpbuf *make_ofp_error_msg(int error, const struct ofp_header *);
+
 #endif /* ofp-util.h */

ofproto/ofproto.c

@@ -2297,34 +2297,15 @@ queue_tx(struct ofpbuf *msg, const struct ofconn *ofconn,
     }
 }
 
-static void
-send_error(const struct ofconn *ofconn, const struct ofp_header *oh,
-           int error, const void *data, size_t len)
-{
-    struct ofpbuf *buf;
-    struct ofp_error_msg *oem;
-
-    if (!(error >> 16)) {
-        VLOG_WARN_RL(&rl, "not sending bad error code %d to controller",
-                     error);
-        return;
-    }
-
-    COVERAGE_INC(ofproto_error);
-    oem = make_openflow_xid(len + sizeof *oem, OFPT_ERROR,
-                            oh ? oh->xid : 0, &buf);
-    oem->type = htons((unsigned int) error >> 16);
-    oem->code = htons(error & 0xffff);
-    memcpy(oem->data, data, len);
-    queue_tx(buf, ofconn, ofconn->reply_counter);
-}
-
 static void
 send_error_oh(const struct ofconn *ofconn, const struct ofp_header *oh,
               int error)
 {
-    size_t oh_length = ntohs(oh->length);
+    struct ofpbuf *buf = make_ofp_error_msg(error, oh);
-    send_error(ofconn, oh, error, oh, MIN(oh_length, 64));
+    if (buf) {
+        COVERAGE_INC(ofproto_error);
+        queue_tx(buf, ofconn, ofconn->reply_counter);
+    }
 }
 
 static void