diff options
| -rw-r--r-- | lib/ofp-errors.c | 9 | ||||
| -rw-r--r-- | ofproto/bundles.c | 22 | ||||
| -rw-r--r-- | ofproto/bundles.h | 24 | ||||
| -rw-r--r-- | ofproto/connmgr.c | 13 | ||||
| -rw-r--r-- | ofproto/connmgr.h | 4 | ||||
| -rw-r--r-- | ofproto/ofproto.c | 5 | ||||
| -rw-r--r-- | tests/ofproto.at | 46 |
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 |