Jarno pointed out that modify_flows__() didn't really need to check every
instance of the flow separately. After some further investigation I
decided that this was even more of an improvement.
CC: Jarno Rajahalme <jrajaha...@nicira.com>
Signed-off-by: Ben Pfaff <b...@nicira.com>
---
lib/ofp-actions.c | 23 ++++++++++++++---------
lib/ofp-actions.h | 5 +++--
lib/ofp-parse.c | 5 +++--
lib/ofp-print.c | 3 ++-
lib/ofp-util.c | 7 +++++--
lib/ofp-util.h | 4 +++-
ofproto/ofproto.c | 50 +++++++++++--------------------------------------
utilities/ovs-ofctl.c | 5 +++--
8 files changed, 44 insertions(+), 58 deletions(-)
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index 6fa80a2..65e0437 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -1565,8 +1565,9 @@ ofpact_check_output_port(ofp_port_t port, ofp_port_t
max_ports)
/* May modify flow->dl_type, caller must restore it. */
static enum ofperr
-ofpact_check__(const struct ofpact *a, struct flow *flow, ofp_port_t max_ports,
- uint8_t table_id, bool enforce_consistency)
+ofpact_check__(const struct ofpact *a, struct flow *flow,
+ bool enforce_consistency, ofp_port_t max_ports,
+ uint8_t table_id, uint8_t n_tables)
{
const struct ofpact_enqueue *enqueue;
@@ -1700,7 +1701,7 @@ ofpact_check__(const struct ofpact *a, struct flow *flow,
ofp_port_t max_ports,
case OFPACT_WRITE_ACTIONS: {
struct ofpact_nest *on = ofpact_get_WRITE_ACTIONS(a);
return ofpacts_check(on->actions, ofpact_nest_get_action_len(on),
- flow, max_ports, table_id, false);
+ flow, false, max_ports, table_id, n_tables);
}
case OFPACT_WRITE_METADATA:
@@ -1714,11 +1715,14 @@ ofpact_check__(const struct ofpact *a, struct flow
*flow, ofp_port_t max_ports,
return 0;
}
- case OFPACT_GOTO_TABLE:
- if (ofpact_get_GOTO_TABLE(a)->table_id <= table_id) {
+ case OFPACT_GOTO_TABLE: {
+ uint8_t goto_table = ofpact_get_GOTO_TABLE(a)->table_id;
+ if ((table_id != 255 && goto_table <= table_id)
+ || (n_tables != 255 && goto_table >= n_tables)) {
return OFPERR_OFPBRC_BAD_TABLE_ID;
}
return 0;
+ }
case OFPACT_GROUP:
return 0;
@@ -1741,16 +1745,17 @@ ofpact_check__(const struct ofpact *a, struct flow
*flow, ofp_port_t max_ports,
* May temporarily modify 'flow', but restores the changes before returning. */
enum ofperr
ofpacts_check(const struct ofpact ofpacts[], size_t ofpacts_len,
- struct flow *flow, ofp_port_t max_ports, uint8_t table_id,
- bool enforce_consistency)
+ struct flow *flow, bool enforce_consistency,
+ ofp_port_t max_ports,
+ uint8_t table_id, uint8_t n_tables)
{
const struct ofpact *a;
ovs_be16 dl_type = flow->dl_type;
enum ofperr error = 0;
OFPACT_FOR_EACH (a, ofpacts, ofpacts_len) {
- error = ofpact_check__(a, flow, max_ports, table_id,
- enforce_consistency);
+ error = ofpact_check__(a, flow, enforce_consistency,
+ max_ports, table_id, n_tables);
if (error) {
break;
}
diff --git a/lib/ofp-actions.h b/lib/ofp-actions.h
index dd8f35f..2312121 100644
--- a/lib/ofp-actions.h
+++ b/lib/ofp-actions.h
@@ -564,8 +564,9 @@ enum ofperr ofpacts_pull_openflow11_instructions(struct
ofpbuf *openflow,
unsigned int instructions_len,
struct ofpbuf *ofpacts);
enum ofperr ofpacts_check(const struct ofpact[], size_t ofpacts_len,
- struct flow *, ofp_port_t max_ports,
- uint8_t table_id, bool enforce_consistency);
+ struct flow *, bool enforce_consistency,
+ ofp_port_t max_ports,
+ uint8_t table_id, uint8_t n_tables);
enum ofperr ofpacts_verify(const struct ofpact ofpacts[], size_t ofpacts_len);
enum ofperr ofpact_check_output_port(ofp_port_t port, ofp_port_t max_ports);
diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
index 757b971..0abd068 100644
--- a/lib/ofp-parse.c
+++ b/lib/ofp-parse.c
@@ -1361,7 +1361,7 @@ parse_ofp_str__(struct ofputil_flow_mod *fm, int command,
char *string,
enum ofperr err;
err = ofpacts_check(ofpacts.data, ofpacts.size, &fm->match.flow,
- OFPP_MAX, 0, true);
+ true, OFPP_MAX, fm->table_id, 255);
if (err) {
if (!enforce_consistency &&
err == OFPERR_OFPBAC_MATCH_INCONSISTENT) {
@@ -1370,7 +1370,8 @@ parse_ofp_str__(struct ofputil_flow_mod *fm, int command,
char *string,
/* Try again, allowing for inconsistency.
* XXX: As a side effect, logging may be duplicated. */
err = ofpacts_check(ofpacts.data, ofpacts.size,
- &fm->match.flow, OFPP_MAX, 0, false);
+ &fm->match.flow, false,
+ OFPP_MAX, fm->table_id, 255);
}
if (err) {
error = xasprintf("actions are invalid with specified
match "
diff --git a/lib/ofp-print.c b/lib/ofp-print.c
index 1dc4598..37e1f4f 100644
--- a/lib/ofp-print.c
+++ b/lib/ofp-print.c
@@ -757,7 +757,8 @@ ofp_print_flow_mod(struct ds *s, const struct ofp_header
*oh, int verbosity)
protocol = ofputil_protocol_set_tid(protocol, true);
ofpbuf_init(&ofpacts, 64);
- error = ofputil_decode_flow_mod(&fm, oh, protocol, &ofpacts);
+ error = ofputil_decode_flow_mod(&fm, oh, protocol, &ofpacts,
+ OFPP_MAX, 255);
if (error) {
ofpbuf_uninit(&ofpacts);
ofp_print_error(s, error);
diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index e6a9494..e78cac2 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -1484,7 +1484,8 @@ enum ofperr
ofputil_decode_flow_mod(struct ofputil_flow_mod *fm,
const struct ofp_header *oh,
enum ofputil_protocol protocol,
- struct ofpbuf *ofpacts)
+ struct ofpbuf *ofpacts,
+ ofp_port_t max_port, uint8_t max_table)
{
ovs_be16 raw_flags;
enum ofperr error;
@@ -1661,7 +1662,9 @@ ofputil_decode_flow_mod(struct ofputil_flow_mod *fm,
: OFPERR_OFPFMFC_TABLE_FULL);
}
- return 0;
+ return ofpacts_check(fm->ofpacts, fm->ofpacts_len, &fm->match.flow,
+ oh->version > OFP10_VERSION, max_port,
+ fm->table_id, max_table);
}
static enum ofperr
diff --git a/lib/ofp-util.h b/lib/ofp-util.h
index a77c301..cf36dce 100644
--- a/lib/ofp-util.h
+++ b/lib/ofp-util.h
@@ -293,7 +293,9 @@ struct ofputil_flow_mod {
enum ofperr ofputil_decode_flow_mod(struct ofputil_flow_mod *,
const struct ofp_header *,
enum ofputil_protocol,
- struct ofpbuf *ofpacts);
+ struct ofpbuf *ofpacts,
+ ofp_port_t max_port,
+ uint8_t max_table);
struct ofpbuf *ofputil_encode_flow_mod(const struct ofputil_flow_mod *,
enum ofputil_protocol);
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 76baaa4..7888bc9 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -2817,26 +2817,14 @@ reject_slave_controller(struct ofconn *ofconn)
}
}
-/* Checks that the 'ofpacts_len' bytes of actions in 'ofpacts' are appropriate
- * for a packet with the prerequisites satisfied by 'flow' in table 'table_id'.
- * 'flow' may be temporarily modified, but is restored at return.
- */
+/* Checks that, if the 'ofpacts_len' bytes of action in 'ofpacts' use a meter,
+ * then that meter is one configured in 'ofproto'. */
static enum ofperr
-ofproto_check_ofpacts(struct ofproto *ofproto,
- const struct ofpact ofpacts[], size_t ofpacts_len,
- struct flow *flow, uint8_t table_id,
- const struct ofp_header *oh)
+ofproto_check_meter(struct ofproto *ofproto,
+ const struct ofpact ofpacts[], size_t ofpacts_len)
{
- enum ofperr error;
uint32_t mid;
- error = ofpacts_check(ofpacts, ofpacts_len, flow,
- u16_to_ofp(ofproto->max_ports), table_id,
- oh && oh->version > OFP10_VERSION);
- if (error) {
- return error;
- }
-
mid = ofpacts_get_meter(ofpacts, ofpacts_len);
if (mid && get_provider_meter_id(ofproto, mid) == UINT32_MAX) {
return OFPERR_OFPMMFC_INVALID_METER;
@@ -2890,7 +2878,7 @@ handle_packet_out(struct ofconn *ofconn, const struct
ofp_header *oh)
/* Verify actions against packet, then send packet if successful. */
in_port_.ofp_port = po.in_port;
flow_extract(payload, 0, 0, NULL, &in_port_, &flow);
- error = ofproto_check_ofpacts(p, po.ofpacts, po.ofpacts_len, &flow, 0, oh);
+ error = ofproto_check_meter(p, po.ofpacts, po.ofpacts_len);
if (!error) {
error = p->ofproto_class->packet_out(p, payload, &flow,
po.ofpacts, po.ofpacts_len);
@@ -3938,14 +3926,6 @@ add_flow(struct ofproto *ofproto, struct ofconn *ofconn,
}
}
- /* Verify actions. */
- error = ofproto_check_ofpacts(ofproto, fm->ofpacts, fm->ofpacts_len,
- &fm->match.flow, table_id, request);
- if (error) {
- cls_rule_destroy(&cr);
- return error;
- }
-
/* Serialize against pending deletion. */
if (is_flow_deletion_pending(ofproto, &cr, table_id)) {
cls_rule_destroy(&cr);
@@ -4044,19 +4024,6 @@ modify_flows__(struct ofproto *ofproto, struct ofconn
*ofconn,
enum ofperr error;
size_t i;
- /* Verify actions before we start to modify any rules, to avoid partial
- * flow table modifications. */
- for (i = 0; i < rules->n; i++) {
- struct rule *rule = rules->rules[i];
-
- error = ofproto_check_ofpacts(ofproto, fm->ofpacts, fm->ofpacts_len,
- &fm->match.flow, rule->table_id,
- request);
- if (error) {
- return error;
- }
- }
-
type = fm->command == OFPFC_ADD ? OFOPERATION_REPLACE : OFOPERATION_MODIFY;
group = ofopgroup_create(ofproto, ofconn, request, fm->buffer_id);
error = OFPERR_OFPBRC_EPERM;
@@ -4389,7 +4356,12 @@ handle_flow_mod(struct ofconn *ofconn, const struct
ofp_header *oh)
ofpbuf_use_stub(&ofpacts, ofpacts_stub, sizeof ofpacts_stub);
error = ofputil_decode_flow_mod(&fm, oh, ofconn_get_protocol(ofconn),
- &ofpacts);
+ &ofpacts,
+ u16_to_ofp(ofproto->max_ports),
+ ofproto->n_tables);
+ if (!error) {
+ error = ofproto_check_meter(ofproto, fm.ofpacts, fm.ofpacts_len);
+ }
if (!error) {
error = handle_flow_mod__(ofproto, ofconn, &fm, oh);
}
diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
index da0a54b..96c1b7b 100644
--- a/utilities/ovs-ofctl.c
+++ b/utilities/ovs-ofctl.c
@@ -3074,8 +3074,9 @@ ofctl_parse_ofp11_instructions(int argc OVS_UNUSED, char
*argv[] OVS_UNUSED)
/* Verify actions, enforce consistency. */
struct flow flow;
memset(&flow, 0, sizeof flow);
- error = ofpacts_check(ofpacts.data, ofpacts.size, &flow, OFPP_MAX,
- table_id ? atoi(table_id) : 0, true);
+ error = ofpacts_check(ofpacts.data, ofpacts.size, &flow,
+ true, OFPP_MAX,
+ table_id ? atoi(table_id) : 0, 255);
}
if (error) {
printf("bad OF1.1 instructions: %s\n\n", ofperr_get_name(error));
--
1.7.10.4
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
