ofproto: Improve OFPT_FLOW_MOD error reporting.

Error reporting to the controller for "flow_mod" operations was poor
because WDP was reporting errors as Unix errno values and nothing was
translating them into OpenFlow errors to send to the controller.  This
commit fixes the problem by making the functions in question instead use
OpenFlow error numbers and, in cases where there is no appropriate number,
using a vendor extension error number.

diff --git a/include/openflow/nicira-ext.h b/include/openflow/nicira-ext.h
index 612d071..9a47718 100644 (file)
--- a/include/openflow/nicira-ext.h
+++ b/include/openflow/nicira-ext.h
@@ -66,6 +66,24 @@ struct nx_vendor_error {
     /* Followed by at least the first 64 bytes of the failed request. */
 };
 \f
+/* Specific Nicira extension error numbers.
+ *
+ * These are the "code" values used in nx_vendor_error.  So far, the "type"
+ * values in nx_vendor_error are the same as those in ofp_error_msg.  That is,
+ * at Nicira so far we've only needed additional vendor-specific 'code' values,
+ * so we're using the existing 'type' values to avoid having to invent new ones
+ * that duplicate the current ones' meanings. */
+
+/* Additional "code" values for OFPET_FLOW_MOD_FAILED. */
+enum {
+    /* Generic hardware error. */
+    NXFMFC_HARDWARE = 0x100,
+
+    /* A nonexistent table ID was specified in the "command" field of struct
+     * ofp_flow_mod, when the nxt_flow_mod_table_id extension is enabled. */
+    NXFMFC_BAD_TABLE_ID
+};
+\f
 /* Nicira vendor requests and replies. */
 
 enum nicira_type {

diff --git a/ofproto/wdp-provider.h b/ofproto/wdp-provider.h
index fc86bd6..d661013 100644 (file)
--- a/ofproto/wdp-provider.h
+++ b/ofproto/wdp-provider.h
@@ -367,8 +367,7 @@ struct wdp_class {
      *         are updated from 'put->idle_timeout' and 'put->hard_timeout',
      *         respectively.
      *
-     * Returns 0 if successful, otherwise a positive errno value.  If
-     * successful:
+     * If successful, returns 0 and:
      *
      *   - If 'old_stats' is nonnull, then 'old_stats' is filled with the
      *     flow's stats as they existed just before the update, or it is zeroed
@@ -376,22 +375,40 @@ struct wdp_class {
      *
      *   - If 'rulep' is nonnull, then it is set to the newly created rule.
      *
-     * Some error return values have specific meanings:
+     * On failure, ordinarily returns an OpenFlow error code constructed with
+     * e.g. ofp_mkerr(), that will be reported to the controller.  Some
+     * examples:
      *
-     *   - ENOENT: Flow does not exist and WDP_PUT_CREATE not specified.
+     *   - ofp_mkerr(OFPET_FLOW_MOD_FAILED, OFPFMFC_ALL_TABLES_FULL): Flow
+     *     table full.
      *
-     *   - EEXIST: Flow exists and WDP_PUT_MODIFY not specified.
+     *   - ofp_mkerr(OFPET_BAD_ACTION, OFPBAC_BAD_OUT_PORT): Output to
+     *     unsupported output port.
+     *
+     *   - ofp_mkerr(OFPET_FLOW_MOD_FAILED, OFPFMFC_UNSUPPORTED): The flow
+     *     table supports the supplied actions but not in the supplied order or
+     *     combination.
+     *
+     *   - OpenFlow lacks appropriate error types and codes for many
+     *     situations.  Feel free to add a new error vendor extension to
+     *     nicira-ext.h to handle these situations with your own vendor ID.  If
+     *     it is a reasonably generic error and you want to use the Nicira
+     *     vendor ID instead of your own, please coordinate with
+     *     dev@openvswitch.org.
      *
-     *   - ENOBUFS: Flow table full.
+     * The following specific kinds of failures should instead be reported as
+     * errno values, for the caller to interpret, instead of the controller:
      *
-     *   - EINVAL: Flow table cannot accept flow of this form.
+     *   - ENOENT: Flow does not exist and WDP_PUT_CREATE not specified.
+     *
+     *   - EEXIST: Flow exists and WDP_PUT_MODIFY not specified.
      */
     int (*flow_put)(struct wdp *wdp, const struct wdp_flow_put *put,
                     struct wdp_flow_stats *old_stats,
                     struct wdp_rule **rulep);
 
-    /* Deletes 'rule' from 'wdp'.  Returns 0 if successful, otherwise a
-     * positive errno value.
+    /* Deletes 'rule' from 'wdp'.  Returns 0 if successful, otherwise an
+     * OpenFlow error code.
      *
      * If successful and 'final_stats' is non-null, stores the flow's
      * statistics just before it is deleted into '*final_stats'. */
@@ -405,7 +422,9 @@ struct wdp_class {
     /* Performs the actions for 'rule' on the Ethernet frame specified in
      * 'packet'.  Pretends that the frame was originally received on the port
      * numbered 'in_port'.  Packets and bytes sent should be credited to
-     * 'rule'. */
+     * 'rule'.
+     *
+     * Returns 0 if successful, otherwise an OpenFlow error code. */
     int (*flow_inject)(struct wdp *wdp, struct wdp_rule *rule,
                        uint16_t in_port, const struct ofpbuf *packet);
 

diff --git a/ofproto/wdp-xflow.c b/ofproto/wdp-xflow.c
index 494aab8..b5f3582 100644 (file)
--- a/ofproto/wdp-xflow.c
+++ b/ofproto/wdp-xflow.c
@@ -1722,7 +1722,7 @@ wx_flow_put(struct wdp *wdp, const struct wdp_flow_put *put,
 
     ofp_table_id = put->flow->wildcards ? TABLEID_CLASSIFIER : TABLEID_HASH;
     if (put->ofp_table_id != 0xff && put->ofp_table_id != ofp_table_id) {
-        return EINVAL;
+        return ofp_mkerr_nicira(OFPET_FLOW_MOD_FAILED, NXFMFC_BAD_TABLE_ID);
     }
 
     rule = wx_rule_cast(classifier_find_rule_exactly(&wx->cls, put->flow));
@@ -1742,7 +1742,7 @@ wx_flow_put(struct wdp *wdp, const struct wdp_flow_put *put,
              ? classifier_count_wild(&wx->cls) >= WX_MAX_WILD
              : classifier_count_exact(&wx->cls) >= WX_MAX_EXACT)) {
             /* XXX subrules should not count against exact-match limit */
-            return ENOBUFS;
+            return ofp_mkerr(OFPET_FLOW_MOD_FAILED, OFPFMFC_ALL_TABLES_FULL);
         }
     }
 
@@ -1821,9 +1821,9 @@ wx_execute(struct wdp *wdp, uint16_t in_port,
     if (error) {
         return error;
     }
-    xfif_execute(wx->xfif, ofp_port_to_xflow_port(in_port),
-                 xflow_actions.actions, xflow_actions.n_actions, packet);
-    return 0;
+    return xfif_execute(wx->xfif, ofp_port_to_xflow_port(in_port),
+                        xflow_actions.actions, xflow_actions.n_actions,
+                        packet);
 }
 
 static int