[ovs-dev] [PATCH 4/5] nx-match: Do not check pre-requisites for load actions
Simon Horman
horms at verge.net.au
Fri Oct 5 08:05:55 UTC 2012
There are (or at least will be) cases where this check can produce false
positives. For example, a flow which matches a non-MPLS packet and then
applies an MPLS push action followed by an action to load the MPLS label.
I believe this is the same problem that was recently discussed in
relation to set-field. In that case my understanding is that consensus
was to omit the check and potentially revisit the problem later.
Signed-off-by: Simon Horman <horms at verge.net.au>
---
lib/autopath.c | 6 +++---
lib/autopath.h | 3 +--
lib/bundle.c | 7 +++----
lib/bundle.h | 3 +--
lib/learn.c | 10 +++++-----
lib/learn.h | 2 +-
lib/meta-flow.c | 14 +++++---------
lib/meta-flow.h | 4 ++--
lib/multipath.c | 7 +++----
lib/multipath.h | 3 +--
lib/nx-match.c | 16 ++++++++--------
lib/nx-match.h | 6 ++----
lib/ofp-actions.c | 23 +++++++++++------------
lib/ofp-actions.h | 2 +-
ofproto/ofproto.c | 4 ++--
15 files changed, 49 insertions(+), 61 deletions(-)
diff --git a/lib/autopath.c b/lib/autopath.c
index b204e84..1af5cd9 100644
--- a/lib/autopath.c
+++ b/lib/autopath.c
@@ -80,16 +80,16 @@ autopath_from_openflow(const struct nx_action_autopath *nap,
return OFPERR_OFPBAC_BAD_ARGUMENT;
}
- return autopath_check(autopath, NULL);
+ return autopath_check(autopath);
}
enum ofperr
-autopath_check(const struct ofpact_autopath *autopath, const struct flow *flow)
+autopath_check(const struct ofpact_autopath *autopath)
{
VLOG_WARN_ONCE("The autopath action is deprecated and may be removed in"
" February 2013. Please email dev at openvswitch.org with"
" concerns.");
- return mf_check_dst(&autopath->dst, flow);
+ return mf_check_dst(&autopath->dst);
}
void
diff --git a/lib/autopath.h b/lib/autopath.h
index 337e7d1..ef46456 100644
--- a/lib/autopath.h
+++ b/lib/autopath.h
@@ -33,8 +33,7 @@ void autopath_parse(struct ofpact_autopath *, const char *);
enum ofperr autopath_from_openflow(const struct nx_action_autopath *,
struct ofpact_autopath *);
-enum ofperr autopath_check(const struct ofpact_autopath *,
- const struct flow *);
+enum ofperr autopath_check(const struct ofpact_autopath *);
void autopath_to_nxast(const struct ofpact_autopath *,
struct ofpbuf *openflow);
diff --git a/lib/bundle.c b/lib/bundle.c
index b68ebab..6ced9ad 100644
--- a/lib/bundle.c
+++ b/lib/bundle.c
@@ -173,20 +173,19 @@ bundle_from_openflow(const struct nx_action_bundle *nab,
ofpact_update_len(ofpacts, &bundle->ofpact);
if (!error) {
- error = bundle_check(bundle, OFPP_MAX, NULL);
+ error = bundle_check(bundle, OFPP_MAX);
}
return error;
}
enum ofperr
-bundle_check(const struct ofpact_bundle *bundle, int max_ports,
- const struct flow *flow)
+bundle_check(const struct ofpact_bundle *bundle, int max_ports)
{
static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
size_t i;
if (bundle->dst.field) {
- enum ofperr error = mf_check_dst(&bundle->dst, flow);
+ enum ofperr error = mf_check_dst(&bundle->dst);
if (error) {
return error;
}
diff --git a/lib/bundle.h b/lib/bundle.h
index 5b6bb67..a41f3f6 100644
--- a/lib/bundle.h
+++ b/lib/bundle.h
@@ -39,8 +39,7 @@ uint16_t bundle_execute(const struct ofpact_bundle *, const struct flow *,
void *aux);
enum ofperr bundle_from_openflow(const struct nx_action_bundle *,
struct ofpbuf *ofpact);
-enum ofperr bundle_check(const struct ofpact_bundle *, int max_ports,
- const struct flow *);
+enum ofperr bundle_check(const struct ofpact_bundle *, int max_ports);
void bundle_to_nxast(const struct ofpact_bundle *, struct ofpbuf *of10);
void bundle_parse(const char *, struct ofpbuf *ofpacts);
void bundle_parse_load(const char *, struct ofpbuf *ofpacts);
diff --git a/lib/learn.c b/lib/learn.c
index b9bbc97..f9d5527 100644
--- a/lib/learn.c
+++ b/lib/learn.c
@@ -168,7 +168,7 @@ learn_from_openflow(const struct nx_action_learn *nal, struct ofpbuf *ofpacts)
/* Checks that 'learn' is a valid action on 'flow'. Returns 0 if it is valid,
* otherwise an OFPERR_*. */
enum ofperr
-learn_check(const struct ofpact_learn *learn, const struct flow *flow)
+learn_check(const struct ofpact_learn *learn)
{
const struct ofpact_learn_spec *spec;
struct match match;
@@ -179,7 +179,7 @@ learn_check(const struct ofpact_learn *learn, const struct flow *flow)
/* Check the source. */
if (spec->src_type == NX_LEARN_SRC_FIELD) {
- error = mf_check_src(&spec->src, flow);
+ error = mf_check_src(&spec->src);
if (error) {
return error;
}
@@ -188,7 +188,7 @@ learn_check(const struct ofpact_learn *learn, const struct flow *flow)
/* Check the destination. */
switch (spec->dst_type) {
case NX_LEARN_DST_MATCH:
- error = mf_check_src(&spec->dst, &match.flow);
+ error = mf_check_src(&spec->dst);
if (error) {
return error;
}
@@ -197,7 +197,7 @@ learn_check(const struct ofpact_learn *learn, const struct flow *flow)
break;
case NX_LEARN_DST_LOAD:
- error = mf_check_dst(&spec->dst, &match.flow);
+ error = mf_check_dst(&spec->dst);
if (error) {
return error;
}
@@ -583,7 +583,7 @@ learn_parse(char *arg, const struct flow *flow, struct ofpbuf *ofpacts)
/* In theory the above should have caught any errors, but... */
if (flow) {
- error = learn_check(learn, flow);
+ error = learn_check(learn);
if (error) {
ovs_fatal(0, "%s: %s", orig, ofperr_to_string(error));
}
diff --git a/lib/learn.h b/lib/learn.h
index adf597e..3fa2d53 100644
--- a/lib/learn.h
+++ b/lib/learn.h
@@ -33,7 +33,7 @@ struct nx_action_learn;
enum ofperr learn_from_openflow(const struct nx_action_learn *,
struct ofpbuf *ofpacts);
-enum ofperr learn_check(const struct ofpact_learn *, const struct flow *);
+enum ofperr learn_check(const struct ofpact_learn *);
void learn_to_nxast(const struct ofpact_learn *, struct ofpbuf *openflow);
void learn_execute(const struct ofpact_learn *, const struct flow *,
diff --git a/lib/meta-flow.c b/lib/meta-flow.c
index 38c9a27..0a5d1ac 100644
--- a/lib/meta-flow.c
+++ b/lib/meta-flow.c
@@ -1670,8 +1670,7 @@ mf_set(const struct mf_field *mf,
}
static enum ofperr
-mf_check__(const struct mf_subfield *sf, const struct flow *flow,
- const char *type)
+mf_check__(const struct mf_subfield *sf, const char *type)
{
if (!sf->field) {
VLOG_WARN_RL(&rl, "unknown %s field", type);
@@ -1684,9 +1683,6 @@ mf_check__(const struct mf_subfield *sf, const struct flow *flow,
VLOG_WARN_RL(&rl, "bit offset %d and width %d exceeds %d-bit width "
"of %s field %s", sf->ofs, sf->n_bits,
sf->field->n_bits, type, sf->field->name);
- } else if (flow && !mf_are_prereqs_ok(sf->field, flow)) {
- VLOG_WARN_RL(&rl, "%s field %s lacks correct prerequisites",
- type, sf->field->name);
} else {
return 0;
}
@@ -1698,18 +1694,18 @@ mf_check__(const struct mf_subfield *sf, const struct flow *flow,
* 0 if so, otherwise an OpenFlow error code (e.g. as returned by
* ofp_mkerr()). */
enum ofperr
-mf_check_src(const struct mf_subfield *sf, const struct flow *flow)
+mf_check_src(const struct mf_subfield *sf)
{
- return mf_check__(sf, flow, "source");
+ return mf_check__(sf, "source");
}
/* Checks whether 'sf' is valid for writing a subfield into 'flow'. Returns 0
* if so, otherwise an OpenFlow error code (e.g. as returned by
* ofp_mkerr()). */
enum ofperr
-mf_check_dst(const struct mf_subfield *sf, const struct flow *flow)
+mf_check_dst(const struct mf_subfield *sf)
{
- int error = mf_check__(sf, flow, "destination");
+ int error = mf_check__(sf, "destination");
if (!error && !sf->field->writable) {
VLOG_WARN_RL(&rl, "destination field %s is not writable",
sf->field->name);
diff --git a/lib/meta-flow.h b/lib/meta-flow.h
index 60bfeca..3fe9014 100644
--- a/lib/meta-flow.h
+++ b/lib/meta-flow.h
@@ -332,8 +332,8 @@ void mf_format_subfield(const struct mf_subfield *, struct ds *);
char *mf_parse_subfield__(struct mf_subfield *sf, const char **s);
const char *mf_parse_subfield(struct mf_subfield *, const char *);
-enum ofperr mf_check_src(const struct mf_subfield *, const struct flow *);
-enum ofperr mf_check_dst(const struct mf_subfield *, const struct flow *);
+enum ofperr mf_check_src(const struct mf_subfield *);
+enum ofperr mf_check_dst(const struct mf_subfield *);
/* Parsing and formatting. */
char *mf_parse(const struct mf_field *, const char *,
diff --git a/lib/multipath.c b/lib/multipath.c
index f6a1a0a..200d2d3 100644
--- a/lib/multipath.c
+++ b/lib/multipath.c
@@ -68,16 +68,15 @@ multipath_from_openflow(const struct nx_action_multipath *nam,
return OFPERR_OFPBAC_BAD_ARGUMENT;
}
- return multipath_check(mp, NULL);
+ return multipath_check(mp);
}
/* Checks that 'mp' is valid on flow. Returns 0 if it is valid, otherwise an
* OFPERR_*. */
enum ofperr
-multipath_check(const struct ofpact_multipath *mp,
- const struct flow *flow)
+multipath_check(const struct ofpact_multipath *mp)
{
- return mf_check_dst(&mp->dst, flow);
+ return mf_check_dst(&mp->dst);
}
/* Converts 'mp' into an OpenFlow NXAST_MULTIPATH action, which it appends to
diff --git a/lib/multipath.h b/lib/multipath.h
index 1b5160d..1f02e9e 100644
--- a/lib/multipath.h
+++ b/lib/multipath.h
@@ -33,8 +33,7 @@ struct ofpbuf;
enum ofperr multipath_from_openflow(const struct nx_action_multipath *,
struct ofpact_multipath *);
-enum ofperr multipath_check(const struct ofpact_multipath *,
- const struct flow *);
+enum ofperr multipath_check(const struct ofpact_multipath *);
void multipath_to_nxast(const struct ofpact_multipath *,
struct ofpbuf *openflow);
diff --git a/lib/nx-match.c b/lib/nx-match.c
index 4254747..0b513c1 100644
--- a/lib/nx-match.c
+++ b/lib/nx-match.c
@@ -1067,7 +1067,7 @@ nxm_reg_move_from_openflow(const struct nx_action_reg_move *narm,
move->dst.ofs = ntohs(narm->dst_ofs);
move->dst.n_bits = ntohs(narm->n_bits);
- return nxm_reg_move_check(move, NULL);
+ return nxm_reg_move_check(move);
}
enum ofperr
@@ -1089,7 +1089,7 @@ nxm_reg_load_from_openflow(const struct nx_action_reg_load *narl,
return OFPERR_OFPBAC_BAD_ARGUMENT;
}
- return nxm_reg_load_check(load, NULL);
+ return nxm_reg_load_check(load);
}
enum ofperr
@@ -1121,26 +1121,26 @@ nxm_reg_load_from_openflow12_set_field(
load = ofpact_put_REG_LOAD(ofpacts);
ofpact_set_field_init(load, mf, oasf + 1);
- return nxm_reg_load_check(load, NULL);
+ return nxm_reg_load_check(load);
}
enum ofperr
-nxm_reg_move_check(const struct ofpact_reg_move *move, const struct flow *flow)
+nxm_reg_move_check(const struct ofpact_reg_move *move)
{
enum ofperr error;
- error = mf_check_src(&move->src, flow);
+ error = mf_check_src(&move->src);
if (error) {
return error;
}
- return mf_check_dst(&move->dst, NULL);
+ return mf_check_dst(&move->dst);
}
enum ofperr
-nxm_reg_load_check(const struct ofpact_reg_load *load, const struct flow *flow)
+nxm_reg_load_check(const struct ofpact_reg_load *load)
{
- return mf_check_dst(&load->dst, flow);
+ return mf_check_dst(&load->dst);
}
void
diff --git a/lib/nx-match.h b/lib/nx-match.h
index 6a57297..9ece6e8 100644
--- a/lib/nx-match.h
+++ b/lib/nx-match.h
@@ -68,10 +68,8 @@ enum ofperr nxm_reg_load_from_openflow(const struct nx_action_reg_load *,
enum ofperr nxm_reg_load_from_openflow12_set_field(
const struct ofp12_action_set_field * oasf, struct ofpbuf *ofpacts);
-enum ofperr nxm_reg_move_check(const struct ofpact_reg_move *,
- const struct flow *);
-enum ofperr nxm_reg_load_check(const struct ofpact_reg_load *,
- const struct flow *);
+enum ofperr nxm_reg_move_check(const struct ofpact_reg_move *);
+enum ofperr nxm_reg_load_check(const struct ofpact_reg_load *);
void nxm_reg_move_to_nxast(const struct ofpact_reg_move *,
struct ofpbuf *openflow);
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index a58f8db..24ee741 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -109,7 +109,7 @@ output_reg_from_openflow(const struct nx_action_output_reg *naor,
output_reg->src.n_bits = nxm_decode_n_bits(naor->ofs_nbits);
output_reg->max_len = ntohs(naor->max_len);
- return mf_check_src(&output_reg->src, NULL);
+ return mf_check_src(&output_reg->src);
}
static void
@@ -975,7 +975,7 @@ exit:
}
static enum ofperr
-ofpact_check__(const struct ofpact *a, const struct flow *flow, int max_ports)
+ofpact_check__(const struct ofpact *a, int max_ports)
{
const struct ofpact_enqueue *enqueue;
@@ -996,10 +996,10 @@ ofpact_check__(const struct ofpact *a, const struct flow *flow, int max_ports)
return 0;
case OFPACT_OUTPUT_REG:
- return mf_check_src(&ofpact_get_OUTPUT_REG(a)->src, flow);
+ return mf_check_src(&ofpact_get_OUTPUT_REG(a)->src);
case OFPACT_BUNDLE:
- return bundle_check(ofpact_get_BUNDLE(a), max_ports, flow);
+ return bundle_check(ofpact_get_BUNDLE(a), max_ports);
case OFPACT_SET_VLAN_VID:
case OFPACT_SET_VLAN_PCP:
@@ -1014,10 +1014,10 @@ ofpact_check__(const struct ofpact *a, const struct flow *flow, int max_ports)
return 0;
case OFPACT_REG_MOVE:
- return nxm_reg_move_check(ofpact_get_REG_MOVE(a), flow);
+ return nxm_reg_move_check(ofpact_get_REG_MOVE(a));
case OFPACT_REG_LOAD:
- return nxm_reg_load_check(ofpact_get_REG_LOAD(a), flow);
+ return nxm_reg_load_check(ofpact_get_REG_LOAD(a));
case OFPACT_DEC_TTL:
case OFPACT_SET_TUNNEL:
@@ -1028,13 +1028,13 @@ ofpact_check__(const struct ofpact *a, const struct flow *flow, int max_ports)
return 0;
case OFPACT_LEARN:
- return learn_check(ofpact_get_LEARN(a), flow);
+ return learn_check(ofpact_get_LEARN(a));
case OFPACT_MULTIPATH:
- return multipath_check(ofpact_get_MULTIPATH(a), flow);
+ return multipath_check(ofpact_get_MULTIPATH(a));
case OFPACT_AUTOPATH:
- return autopath_check(ofpact_get_AUTOPATH(a), flow);
+ return autopath_check(ofpact_get_AUTOPATH(a));
case OFPACT_NOTE:
case OFPACT_EXIT:
@@ -1049,13 +1049,12 @@ ofpact_check__(const struct ofpact *a, const struct flow *flow, int max_ports)
* appropriate for a packet with the prerequisites satisfied by 'flow' in a
* switch with no more than 'max_ports' ports. */
enum ofperr
-ofpacts_check(const struct ofpact ofpacts[], size_t ofpacts_len,
- const struct flow *flow, int max_ports)
+ofpacts_check(const struct ofpact ofpacts[], size_t ofpacts_len, int max_ports)
{
const struct ofpact *a;
OFPACT_FOR_EACH (a, ofpacts, ofpacts_len) {
- enum ofperr error = ofpact_check__(a, flow, max_ports);
+ enum ofperr error = ofpact_check__(a, max_ports);
if (error) {
return error;
}
diff --git a/lib/ofp-actions.h b/lib/ofp-actions.h
index fd53e62..b68de67 100644
--- a/lib/ofp-actions.h
+++ b/lib/ofp-actions.h
@@ -427,7 +427,7 @@ 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,
- const struct flow *, int max_ports);
+ int max_ports);
/* Converting ofpacts to OpenFlow. */
void ofpacts_put_openflow10(const struct ofpact[], size_t ofpacts_len,
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 52203fd..5ae7299 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -2148,7 +2148,7 @@ handle_packet_out(struct ofconn *ofconn, const struct ofp_header *oh)
/* Verify actions against packet, then send packet if successful. */
flow_extract(payload, 0, 0, po.in_port, &flow);
- error = ofpacts_check(po.ofpacts, po.ofpacts_len, &flow, p->max_ports);
+ error = ofpacts_check(po.ofpacts, po.ofpacts_len, p->max_ports);
if (!error) {
error = p->ofproto_class->packet_out(p, payload, &flow,
po.ofpacts, po.ofpacts_len);
@@ -3307,7 +3307,7 @@ handle_flow_mod(struct ofconn *ofconn, const struct ofp_header *oh)
}
if (!error) {
error = ofpacts_check(fm.ofpacts, fm.ofpacts_len,
- &fm.match.flow, ofproto->max_ports);
+ ofproto->max_ports);
}
if (!error) {
error = handle_flow_mod__(ofconn_get_ofproto(ofconn), ofconn, &fm, oh);
--
1.7.10.4
More information about the dev
mailing list