ofp-errors: Send as much of a message as possible in an error reply.

When an OpenFlow switch sends an error message in reply to a request
from a controller, it includes an excerpt from the original request.  The
OpenFlow specification only requires the switch to send at least the first
64 bytes of the request.  Until now, Open vSwitch has only sent that much.
This commit changes Open vSwitch to instead send the entire message, only
trimming it if necessary to keep the error message within the 64 kB limit
for an OpenFlow message.  This should simplify debugging for controller
authors.

CC: Suneel Bomminayuni <sbomminayuni@vmware.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
Acked-by: Justin Pettit <jpettit@ovn.org>

7 files changed, 40 insertions, 83 deletions

diff --git a/lib/ofp-errors.c b/lib/ofp-errors.c
index adc929517..73930cc56 100644
--- a/lib/ofp-errors.c
+++ b/lib/ofp-errors.c
@@ -203,7 +203,7 @@ ofperr_encode_msg__(enum ofperr error, enum ofp_version ofp_version,
         ofpbuf_put(buf, &vendor, sizeof vendor);
     }
 
-    ofpbuf_put(buf, data, data_len);
+    ofpbuf_put(buf, data, MIN(data_len, UINT16_MAX - buf->size));
     ofpmsg_update_length(buf);
 
     return buf;
@@ -215,16 +215,15 @@ ofperr_encode_msg__(enum ofperr error, enum ofp_version ofp_version,
  * 'oh->version' determines the OpenFlow version of the error reply.
  * 'oh->xid' determines the xid of the error reply.
  * The error reply will contain an initial subsequence of 'oh', up to
- * 'oh->length' or 64 bytes, whichever is shorter.
+ * 'oh->length' (or however much fits).
  *
  * This function isn't appropriate for encoding OFPET_HELLO_FAILED error
  * messages.  Use ofperr_encode_hello() instead. */
 struct ofpbuf *
 ofperr_encode_reply(enum ofperr error, const struct ofp_header *oh)
 {
-    uint16_t len = ntohs(oh->length);
-
-    return ofperr_encode_msg__(error, oh->version, oh->xid, oh, MIN(len, 64));
+    return ofperr_encode_msg__(error, oh->version, oh->xid,
+                               oh, ntohs(oh->length));
 }
 
 /* Creates and returns an OpenFlow message of type OFPT_ERROR that conveys the
diff --git a/ofproto/bundles.c b/ofproto/bundles.c
index 849f99a15..aab15be79 100644
--- a/ofproto/bundles.c
+++ b/ofproto/bundles.c
@@ -51,13 +51,10 @@ ofp_bundle_create(uint32_t id, uint16_t flags, const struct ofp_header *oh)
     bundle->id = id;
     bundle->flags = flags;
     bundle->state = BS_OPEN;
+    bundle->msg = xmemdup(oh, ntohs(oh->length));
 
     ovs_list_init(&bundle->msg_list);
 
-    /* Max 64 bytes for error reporting. */
-    memcpy(bundle->ofp_msg_data, oh,
-           MIN(ntohs(oh->length), sizeof bundle->ofp_msg_data));
-
     return bundle;
 }
 
@@ -71,6 +68,7 @@ ofp_bundle_remove__(struct ofconn *ofconn, struct ofp_bundle *bundle)
     }
 
     ofconn_remove_bundle(ofconn, bundle);
+    free(bundle->msg);
     free(bundle);
 }
 
@@ -79,7 +77,6 @@ ofp_bundle_open(struct ofconn *ofconn, uint32_t id, uint16_t flags,
                 const struct ofp_header *oh)
 {
     struct ofp_bundle *bundle;
-    enum ofperr error;
 
     bundle = ofconn_get_bundle(ofconn, id);
 
@@ -91,12 +88,9 @@ ofp_bundle_open(struct ofconn *ofconn, uint32_t id, uint16_t flags,
     }
 
     bundle = ofp_bundle_create(id, flags, oh);
-    error = ofconn_insert_bundle(ofconn, bundle);
-    if (error) {
-        free(bundle);
-    }
+    ofconn_insert_bundle(ofconn, bundle);
 
-    return error;
+    return 0;
 }
 
 enum ofperr
@@ -150,14 +144,8 @@ ofp_bundle_add_message(struct ofconn *ofconn, uint32_t id, uint16_t flags,
     bundle = ofconn_get_bundle(ofconn, id);
 
     if (!bundle) {
-        enum ofperr error;
-
         bundle = ofp_bundle_create(id, flags, oh);
-        error = ofconn_insert_bundle(ofconn, bundle);
-        if (error) {
-            free(bundle);
-            return error;
-        }
+        ofconn_insert_bundle(ofconn, bundle);
     } else if (bundle->state == BS_CLOSED) {
         ofp_bundle_remove__(ofconn, bundle);
         return OFPERR_OFPBFC_BUNDLE_CLOSED;
diff --git a/ofproto/bundles.h b/ofproto/bundles.h
index b63681708..65ac5cb52 100644
--- a/ofproto/bundles.h
+++ b/ofproto/bundles.h
@@ -35,18 +35,13 @@ struct ofp_bundle_entry {
     struct ovs_list   node;
     enum ofptype      type;  /* OFPTYPE_FLOW_MOD, OFPTYPE_PORT_MOD,
                               * OFPTYPE_GROUP_MOD, OFPTYPE_PACKET_OUT. */
+    struct ofp_header *msg;  /* Original request, for error reporting. */
     union {
         struct ofproto_flow_mod ofm;
         struct ofproto_port_mod opm;
         struct ofproto_group_mod ogm;
         struct ofproto_packet_out opo;
     };
-
-    /* OpenFlow header and some of the message contents for error reporting. */
-    union {
-        struct ofp_header ofp_msg;
-        uint8_t ofp_msg_data[64];
-    };
 };
 
 enum bundle_state {
@@ -60,15 +55,8 @@ struct ofp_bundle {
     uint32_t          id;
     uint16_t          flags;
     enum bundle_state state;
-
-    /* List of 'struct bundle_message's */
-    struct ovs_list   msg_list;
-
-    /* OpenFlow header and some of the message contents for error reporting. */
-    union {
-        struct ofp_header ofp_msg;
-        uint8_t ofp_msg_data[64];
-    };
+    struct ofp_header *msg;      /* Original request, for error reporting. */
+    struct ovs_list   msg_list;  /* List of 'struct bundle_message's */
 };
 
 static inline struct ofp_bundle_entry *ofp_bundle_entry_alloc(
@@ -91,10 +79,7 @@ ofp_bundle_entry_alloc(enum ofptype type, const struct ofp_header *oh)
     struct ofp_bundle_entry *entry = xmalloc(sizeof *entry);
 
     entry->type = type;
-
-    /* Max 64 bytes for error reporting. */
-    memcpy(entry->ofp_msg_data, oh,
-           MIN(ntohs(oh->length), sizeof entry->ofp_msg_data));
+    entry->msg = xmemdup(oh, ntohs(oh->length));
 
     return entry;
 }
@@ -110,6 +95,7 @@ ofp_bundle_entry_free(struct ofp_bundle_entry *entry)
         } else if (entry->type == OFPTYPE_PACKET_OUT) {
             ofproto_packet_out_uninit(&entry->opo);
         }
+        free(entry->msg);
         free(entry);
     }
 }
diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c
index 7f33fbddf..283b80d8d 100644
--- a/ofproto/connmgr.c
+++ b/ofproto/connmgr.c
@@ -1139,8 +1139,7 @@ ofconn_send_replies(const struct ofconn *ofconn, struct ovs_list *replies)
     }
 }
 
-/* Sends 'error' on 'ofconn', as a reply to 'request'.  Only at most the
- * first 64 bytes of 'request' are used. */
+/* Sends 'error' on 'ofconn', as a reply to 'request'. */
 void
 ofconn_send_error(const struct ofconn *ofconn,
                   const struct ofp_header *request, enum ofperr error)
@@ -1222,20 +1221,16 @@ ofconn_get_bundle(struct ofconn *ofconn, uint32_t id)
     return NULL;
 }
 
-enum ofperr
+void
 ofconn_insert_bundle(struct ofconn *ofconn, struct ofp_bundle *bundle)
 {
     hmap_insert(&ofconn->bundles, &bundle->node, bundle_hash(bundle->id));
-
-    return 0;
 }
 
-enum ofperr
+void
 ofconn_remove_bundle(struct ofconn *ofconn, struct ofp_bundle *bundle)
 {
     hmap_remove(&ofconn->bundles, &bundle->node);
-
-    return 0;
 }
 
 static void
@@ -1256,7 +1251,7 @@ bundle_remove_expired(struct ofconn *ofconn, long long int now)
 
     HMAP_FOR_EACH_SAFE (b, next, node, &ofconn->bundles) {
         if (b->used <= limit) {
-            ofconn_send_error(ofconn, &b->ofp_msg, OFPERR_OFPBFC_TIMEOUT);
+            ofconn_send_error(ofconn, b->msg, OFPERR_OFPBFC_TIMEOUT);
             ofp_bundle_remove__(ofconn, b);
         }
     }
diff --git a/ofproto/connmgr.h b/ofproto/connmgr.h
index 1a75c4c85..a3c534d66 100644
--- a/ofproto/connmgr.h
+++ b/ofproto/connmgr.h
@@ -132,8 +132,8 @@ void ofconn_send_error(const struct ofconn *, const struct ofp_header *request,
 struct ofp_bundle;
 
 struct ofp_bundle *ofconn_get_bundle(struct ofconn *, uint32_t id);
-enum ofperr ofconn_insert_bundle(struct ofconn *, struct ofp_bundle *);
-enum ofperr ofconn_remove_bundle(struct ofconn *, struct ofp_bundle *);
+void ofconn_insert_bundle(struct ofconn *, struct ofp_bundle *);
+void ofconn_remove_bundle(struct ofconn *, struct ofp_bundle *);
 
 /* Logging flow_mod summaries. */
 void ofconn_report_flow_mod(struct ofconn *, enum ofp_flow_mod_command);
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index a09dbb2c4..139fcf96e 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -7792,7 +7792,7 @@ do_bundle_commit(struct ofconn *ofconn, uint32_t id, uint16_t flags)
         if (error) {
             /* Send error referring to the original message. */
             if (error) {
-                ofconn_send_error(ofconn, &be->ofp_msg, error);
+                ofconn_send_error(ofconn, be->msg, error);
                 error = OFPERR_OFPBFC_MSG_FAILED;
             }
 
@@ -7834,8 +7834,7 @@ do_bundle_commit(struct ofconn *ofconn, uint32_t id, uint16_t flags)
                             ofproto, ofproto->tables_version);
                     }
 
-                    struct openflow_mod_requester req = { ofconn,
-                                                          &be->ofp_msg };
+                    struct openflow_mod_requester req = { ofconn, be->msg };
 
                     if (be->type == OFPTYPE_FLOW_MOD) {
                         ofproto_flow_mod_finish(ofproto, &be->ofm, &req);
diff --git a/tests/ofproto.at b/tests/ofproto.at
index 5d90a21a0..540928f1d 100644
--- a/tests/ofproto.at
+++ b/tests/ofproto.at
@@ -590,6 +590,7 @@ AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn insert-buckets br0 group_id=1234,comman
 AT_CHECK([strip_xids < stderr | sed '/truncated/,$d'], [0], [dnl
 OFPT_ERROR (OF1.5): OFPGMFC_BUCKET_EXISTS
 OFPT_GROUP_MOD (OF1.5):
+ INSERT_BUCKET command_bucket_id:last,group_id=1234,bucket=bucket_id:14,actions=output:14,bucket=bucket_id:15,actions=output:15
 ])
 
 
@@ -1032,18 +1033,9 @@ udp actions=group:1235
 AT_CHECK([ovs-ofctl -O OpenFlow11 -vwarn add-flow br0 'tcp actions=group:1234'])
 AT_CHECK([ovs-ofctl -O OpenFlow11 -vwarn add-flow br0 'tcp actions=group:1235'], [1], [], [stderr])
 
-# The output should look like this:
-#
-# 00000000  02 0e 00 98 00 00 00 02-00 00 00 00 00 00 00 00 |................|
-# 00000010  00 00 00 00 00 00 00 00-ff 00 00 00 00 00 80 00 |................|
-# 00000020  ff ff ff ff ff ff ff ff-ff ff ff ff 00 00 00 00 |................|
-# 00000030  00 00 00 58 00 00 00 00-00 00 03 d7 00 00 00 00 |...X............|
-#
-# This 'sed' command captures the error message but drops details.
-AT_CHECK([sed '/truncated/d
-/^000000.0/d' stderr | strip_xids], [0],
+AT_CHECK([strip_xids < stderr], [0],
   [OFPT_ERROR (OF1.1): OFPBAC_BAD_OUT_GROUP
-OFPT_FLOW_MOD (OF1.1):
+OFPT_FLOW_MOD (OF1.1): ADD tcp actions=group:1235
 ])
 OVS_VSWITCHD_STOP
 AT_CLEANUP
@@ -1057,16 +1049,7 @@ flow add udp actions=group:1235
 ])
 AT_CHECK([ovs-ofctl -vwarn bundle br0 bundle.txt], [1], [], [stderr])
 
-# The output should look like this:
-#
-# 00000000  02 0e 00 98 00 00 00 02-00 00 00 00 00 00 00 00 |................|
-# 00000010  00 00 00 00 00 00 00 00-ff 00 00 00 00 00 80 00 |................|
-# 00000020  ff ff ff ff ff ff ff ff-ff ff ff ff 00 00 00 00 |................|
-# 00000030  00 00 00 58 00 00 00 00-00 00 03 d7 00 00 00 00 |...X............|
-#
-# This 'sed' command captures the error message but drops details.
-AT_CHECK([sed '/truncated/d
-/^000000.0/d' stderr | ofctl_strip | sed '/talking to/,$d'], [0],
+AT_CHECK([ofctl_strip < stderr | sed '/talking to/,$d'], [0],
   [dnl
 Error OFPBAC_BAD_OUT_GROUP for: OFPT_FLOW_MOD (OF1.4): ADD udp actions=group:1235
 Error OFPBFC_MSG_FAILED for: OFPT_BUNDLE_CONTROL (OF1.4):
@@ -2696,13 +2679,15 @@ NXST_FLOW reply:
 ])
 # Adding another flow will be refused.
 AT_CHECK([ovs-ofctl add-flow br0 in_port=5,actions=drop], [1], [], [stderr])
-AT_CHECK([head -n 1 stderr | ofctl_strip], [0],
+AT_CHECK([ofctl_strip < stderr], [0],
   [OFPT_ERROR: OFPFMFC_TABLE_FULL
+OFPT_FLOW_MOD: ADD in_port=5 actions=drop
 ])
 # Also a mod-flow that would add a flow will be refused.
 AT_CHECK([ovs-ofctl mod-flows br0 in_port=5,actions=drop], [1], [], [stderr])
-AT_CHECK([head -n 1 stderr | ofctl_strip], [0],
+AT_CHECK([ofctl_strip < stderr], [0],
   [OFPT_ERROR: OFPFMFC_TABLE_FULL
+OFPT_FLOW_MOD: MOD in_port=5 actions=drop
 ])
 # Replacing or modifying an existing flow is allowed.
 AT_CHECK([ovs-ofctl add-flow br0 in_port=4,actions=normal])
@@ -2740,8 +2725,9 @@ OFPST_FLOW reply (OF1.2):
 ])
 # Adding another flow will be refused.
 AT_CHECK([ovs-ofctl -O OpenFlow12 add-flow br0 in_port=5,actions=drop], [1], [], [stderr])
-AT_CHECK([head -n 1 stderr | ofctl_strip], [0],
+AT_CHECK([ofctl_strip < stderr], [0],
   [OFPT_ERROR (OF1.2): OFPFMFC_TABLE_FULL
+OFPT_FLOW_MOD (OF1.2): ADD in_port=5 actions=drop
 ])
 # Replacing or modifying an existing flow is allowed.
 AT_CHECK([ovs-ofctl -O OpenFlow12 add-flow br0 in_port=4,actions=normal])
@@ -2801,8 +2787,9 @@ NXST_FLOW reply:
 # Flows with no timeouts at all cannot be evicted.
 AT_CHECK([ovs-ofctl add-flow br0 in_port=7,actions=normal])
 AT_CHECK([ovs-ofctl add-flow br0 in_port=8,actions=drop], [1], [], [stderr])
-AT_CHECK([head -n 1 stderr | ofctl_strip], [0],
+AT_CHECK([ofctl_strip < stderr], [0],
   [OFPT_ERROR: OFPFMFC_TABLE_FULL
+OFPT_FLOW_MOD: ADD in_port=8 actions=drop
 ])
 AT_CHECK([ovs-ofctl dump-flows br0 | ofctl_strip | sort], [0], [dnl
  in_port=4 actions=NORMAL
@@ -2860,8 +2847,9 @@ OFPST_FLOW reply (OF1.2):
 AT_CHECK([ovs-ofctl -O OpenFlow12 add-flow br0 in_port=6,actions=drop])
 AT_CHECK([ovs-ofctl -O OpenFlow12 add-flow br0 in_port=7,actions=normal])
 AT_CHECK([ovs-ofctl -O OpenFlow12 add-flow br0 in_port=8,actions=drop], [1], [], [stderr])
-AT_CHECK([head -n 1 stderr | ofctl_strip], [0],
+AT_CHECK([ofctl_strip < stderr], [0],
   [OFPT_ERROR (OF1.2): OFPFMFC_TABLE_FULL
+OFPT_FLOW_MOD (OF1.2): ADD in_port=8 actions=drop
 ])
 AT_CHECK([ovs-ofctl -O OpenFlow12 dump-flows br0 | ofctl_strip | sort], [0], [dnl
  in_port=4 actions=NORMAL
@@ -2909,8 +2897,9 @@ OFPST_FLOW reply (OF1.4):
 AT_CHECK([ovs-ofctl -O OpenFlow14 mod-table br0 0 noevict])
 # Adding another flow will cause the system to give error for FULL TABLE.
 AT_CHECK([ovs-ofctl -O Openflow14 add-flow br0 hard_timeout=506,importance=36,priority=11,actions=drop],[1], [], [stderr])
-AT_CHECK([head -n 1 stderr | ofctl_strip], [0],
+AT_CHECK([ofctl_strip < stderr], [0],
  [OFPT_ERROR (OF1.4): OFPFMFC_TABLE_FULL
+OFPT_FLOW_MOD (OF1.4): ADD priority=11 hard:506 importance:36 actions=drop
 ])
 #Dump flows. It should show only the old values
 AT_CHECK([ovs-ofctl -O Openflow14 dump-flows br0 | ofctl_strip | sort], [0], [dnl
@@ -2931,8 +2920,9 @@ OFPST_FLOW reply (OF1.4):
 ])
 # Also a mod-flow that would add a flow will be refused.
 AT_CHECK([ovs-ofctl mod-flows br0 in_port=5,actions=drop], [1], [], [stderr])
-AT_CHECK([head -n 1 stderr | ofctl_strip], [0],
+AT_CHECK([ofctl_strip < stderr], [0],
   [OFPT_ERROR: OFPFMFC_TABLE_FULL
+OFPT_FLOW_MOD: MOD in_port=5 actions=drop
 ])
 OVS_VSWITCHD_STOP
 AT_CLEANUP