Skip to content

Commit

Permalink
ofp-errors: Send as much of a message as possible in an error reply.
Browse files Browse the repository at this point in the history 
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 <[email protected]>
Signed-off-by: Ben Pfaff <[email protected]>
Acked-by: Justin Pettit <[email protected]>
  • Loading branch information
@blp
blp committed Jan 9, 2018
1 parent f59cb33 commit 52c57cb
Show file tree
Hide file tree
Showing 7 changed files with 40 additions and 83 deletions.
9 changes: 4 additions & 5 deletions lib/ofp-errors.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
Expand Down
22 changes: 5 additions & 17 deletions ofproto/bundles.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand All @@ -71,6 +68,7 @@ ofp_bundle_remove__(struct ofconn *ofconn, struct ofp_bundle *bundle)
}

ofconn_remove_bundle(ofconn, bundle);
free(bundle->msg);
free(bundle);
}

Expand All @@ -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);

Expand All @@ -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
Expand Down Expand Up @@ -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;
Expand Down
24 changes: 5 additions & 19 deletions ofproto/bundles.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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(
Expand All @@ -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;
}
Expand All @@ -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);
}
}
Expand Down
13 changes: 4 additions & 9 deletions ofproto/connmgr.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand All @@ -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);
}
}
Expand Down
4 changes: 2 additions & 2 deletions ofproto/connmgr.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
5 changes: 2 additions & 3 deletions ofproto/ofproto.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down Expand Up @@ -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);
Expand Down
46 changes: 18 additions & 28 deletions tests/ofproto.at
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
])


Expand Down Expand Up @@ -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
Expand All @@ -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):
Expand Down Expand Up @@ -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])
Expand Down Expand Up @@ -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])
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down

0 comments on commit 52c57cb

Please sign in to comment.