[ovs-dev] [PATCH] ofp-errors: Send as much of a message as possible in an error reply.

Ben Pfaff blp at ovn.org
Sat Jan 6 18:33:14 UTC 2018


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 at vmware.com>
Signed-off-by: Ben Pfaff <blp at ovn.org>
---
 lib/ofp-errors.c  |  9 ++++-----
 ofproto/bundles.c |  6 ++----
 ofproto/bundles.h | 24 +++++-------------------
 ofproto/connmgr.c |  2 +-
 ofproto/ofproto.c |  5 ++---
 tests/ofproto.at  | 46 ++++++++++++++++++----------------------------
 6 files changed, 32 insertions(+), 60 deletions(-)

diff --git a/lib/ofp-errors.c b/lib/ofp-errors.c
index adc929517d89..73930cc56dd0 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 849f99a15e40..195b9f90ef11 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);
 }
 
diff --git a/ofproto/bundles.h b/ofproto/bundles.h
index b63681708d3b..65ac5cb52b90 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 7f33fbddfe3f..ae53bfb04a83 100644
--- a/ofproto/connmgr.c
+++ b/ofproto/connmgr.c
@@ -1256,7 +1256,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/ofproto.c b/ofproto/ofproto.c
index f3946d1afa3f..b5f8a2af579f 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -7780,7 +7780,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;
             }
 
@@ -7822,8 +7822,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 9759450185ac..bb645f6f63ae 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):
@@ -2729,13 +2712,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])
@@ -2773,8 +2758,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])
@@ -2834,8 +2820,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
@@ -2893,8 +2880,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
@@ -2942,8 +2930,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
@@ -2964,8 +2953,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
-- 
2.10.2



More information about the dev mailing list