> On Mar 2, 2017, at 5:34 PM, Joe Stringer <[email protected]> wrote:
>
> On 28 February 2017 at 17:17, Jarno Rajahalme <[email protected]
> <mailto:[email protected]>> wrote:
>> Supply the match mask to prerequisities checking when available. This
>> allows checking for zero-valued matches. Non-zero valued matches
>> imply the presense of corresponding mask bits, but for zero valued
>> matches we must explicitly check the mask, too.
>>
>> This is required now only for conntrack validity checking due to the
>> conntrack state having and 'invalid' bit, but not 'valid' bit. One
>> way to match an valid conntrack state is to match on the 'tracked' bit
>> being one and 'invalid' bit being zero. The latter requires the
>> corresponding mask bit be verified.
>>
>> Signed-off-by: Jarno Rajahalme <[email protected]>
>> ---
>> include/openvswitch/meta-flow.h | 5 ++--
>> include/openvswitch/ofp-actions.h | 4 ++--
>> lib/bundle.c | 4 ++--
>> lib/bundle.h | 3 ++-
>> lib/learn.c | 15 ++++++------
>> lib/learn.h | 3 ++-
>> lib/meta-flow.c | 38 ++++++++++++++++++++++-------
>> lib/multipath.c | 4 ++--
>> lib/multipath.h | 3 ++-
>> lib/nx-match.c | 17 ++++++-------
>> lib/nx-match.h | 6 ++---
>> lib/ofp-actions.c | 50
>> ++++++++++++++++++++-------------------
>> lib/ofp-parse.c | 2 +-
>> lib/ofp-util.c | 2 +-
>> ofproto/ofproto-dpif-trace.c | 13 +++++-----
>> ofproto/ofproto.c | 5 ++--
>> utilities/ovs-ofctl.c | 6 ++---
>> 17 files changed, 105 insertions(+), 75 deletions(-)
>>
>> diff --git a/include/openvswitch/meta-flow.h
>> b/include/openvswitch/meta-flow.h
>> index 83e2599..aac9945 100644
>> --- a/include/openvswitch/meta-flow.h
>> +++ b/include/openvswitch/meta-flow.h
>> @@ -1898,6 +1898,7 @@ void mf_get_mask(const struct mf_field *, const struct
>> flow_wildcards *,
>> /* Prerequisites. */
>> bool mf_are_prereqs_ok(const struct mf_field *mf, const struct flow *flow,
>> struct flow_wildcards *wc);
>> +bool mf_are_match_prereqs_ok(const struct mf_field *, const struct match *);
>>
>> static inline bool
>> mf_is_l3_or_higher(const struct mf_field *mf)
>> @@ -1959,8 +1960,8 @@ void mf_subfield_swap(const struct mf_subfield *,
>> const struct mf_subfield *,
>> struct flow *flow, struct flow_wildcards *);
>>
>> -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 *, const struct match *);
>> +enum ofperr mf_check_dst(const struct mf_subfield *, const struct match *);
>>
>> /* Parsing and formatting. */
>> char *mf_parse(const struct mf_field *, const char *,
>
> I assume that the above is OK from a library standpoint because we are
> not currently guaranteeing that APIs remain stable between release
> versions, only within the minor revisions of a particular release. IE,
> library users compiling against libopenvswitch from 2.7 will need to
> change their code to compile against libopenvswitch from 2.8.
>
I did not remember this, but seems all right to me.
>> diff --git a/include/openvswitch/ofp-actions.h
>> b/include/openvswitch/ofp-actions.h
>> index 88f573d..53d6b44 100644
>> --- a/include/openvswitch/ofp-actions.h
>> +++ b/include/openvswitch/ofp-actions.h
>> @@ -954,11 +954,11 @@ ofpacts_pull_openflow_instructions(struct ofpbuf
>> *openflow,
>> const struct vl_mff_map *vl_mff_map,
>> struct ofpbuf *ofpacts);
>> enum ofperr ofpacts_check(struct ofpact[], size_t ofpacts_len,
>> - struct flow *, ofp_port_t max_ports,
>> + struct match *, ofp_port_t max_ports,
>> uint8_t table_id, uint8_t n_tables,
>> enum ofputil_protocol *usable_protocols);
>> enum ofperr ofpacts_check_consistency(struct ofpact[], size_t ofpacts_len,
>> - struct flow *, ofp_port_t max_ports,
>> + struct match *, ofp_port_t max_ports,
>> uint8_t table_id, uint8_t n_tables,
>> enum ofputil_protocol
>> usable_protocols);
>> enum ofperr ofpact_check_output_port(ofp_port_t port, ofp_port_t max_ports);
>> diff --git a/lib/bundle.c b/lib/bundle.c
>> index 70a743b..620318e 100644
>> --- a/lib/bundle.c
>> +++ b/lib/bundle.c
>> @@ -105,13 +105,13 @@ bundle_execute(const struct ofpact_bundle *bundle,
>>
>> enum ofperr
>> bundle_check(const struct ofpact_bundle *bundle, ofp_port_t max_ports,
>> - const struct flow *flow)
>> + const struct match *match)
>> {
>> 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, match);
>> if (error) {
>> return error;
>> }
>> diff --git a/lib/bundle.h b/lib/bundle.h
>> index f5ce321..48b9b79 100644
>> --- a/lib/bundle.h
>> +++ b/lib/bundle.h
>> @@ -29,6 +29,7 @@
>> struct ds;
>> struct flow;
>> struct flow_wildcards;
>> +struct match;
>> struct ofpact_bundle;
>> struct ofpbuf;
>>
>> @@ -43,7 +44,7 @@ ofp_port_t bundle_execute(const struct ofpact_bundle *,
>> const struct flow *,
>> bool (*slave_enabled)(ofp_port_t ofp_port, void
>> *aux),
>> void *aux);
>> enum ofperr bundle_check(const struct ofpact_bundle *, ofp_port_t max_ports,
>> - const struct flow *);
>> + const struct match *);
>> char *bundle_parse(const char *, struct ofpbuf *ofpacts)
>> OVS_WARN_UNUSED_RESULT;
>> char *bundle_parse_load(const char *, struct ofpbuf *ofpacts)
>> OVS_WARN_UNUSED_RESULT;
>> diff --git a/lib/learn.c b/lib/learn.c
>> index ce52c35..1990849 100644
>> --- a/lib/learn.c
>> +++ b/lib/learn.c
>> @@ -35,18 +35,18 @@
>> /* 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 match *src_match)
>> {
>> const struct ofpact_learn_spec *spec;
>> - struct match match;
>> + struct match dst_match;
>>
>> - match_init_catchall(&match);
>> + match_init_catchall(&dst_match);
>> OFPACT_LEARN_SPEC_FOR_EACH (spec, learn) {
>> enum ofperr error;
>>
>> /* Check the source. */
>> if (spec->src_type == NX_LEARN_SRC_FIELD) {
>> - error = mf_check_src(&spec->src, flow);
>> + error = mf_check_src(&spec->src, src_match);
>> if (error) {
>> return error;
>> }
>> @@ -55,18 +55,19 @@ 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, &dst_match);
>> if (error) {
>> return error;
>> }
>> if (spec->src_type & NX_LEARN_SRC_IMMEDIATE) {
>> mf_write_subfield_value(&spec->dst,
>> - ofpact_learn_spec_imm(spec),
>> &match);
>> + ofpact_learn_spec_imm(spec),
>> + &dst_match);
>> }
>> break;
>>
>> case NX_LEARN_DST_LOAD:
>> - error = mf_check_dst(&spec->dst, &match.flow);
>> + error = mf_check_dst(&spec->dst, &dst_match);
>> if (error) {
>> return error;
>> }
>> diff --git a/lib/learn.h b/lib/learn.h
>> index adbbdf3..9dd41d3 100644
>> --- a/lib/learn.h
>> +++ b/lib/learn.h
>> @@ -23,6 +23,7 @@
>> struct ds;
>> struct flow;
>> struct flow_wildcards;
>> +struct match;
>> struct ofpbuf;
>> struct ofpact_learn;
>> struct ofputil_flow_mod;
>> @@ -33,7 +34,7 @@ struct nx_action_learn;
>> * See lib/ofp-actions.c for NXAST_LEARN specification.
>> */
>>
>> -enum ofperr learn_check(const struct ofpact_learn *, const struct flow *);
>> +enum ofperr learn_check(const struct ofpact_learn *, const struct match *);
>> void learn_execute(const struct ofpact_learn *, const struct flow *,
>> struct ofputil_flow_mod *, struct ofpbuf *ofpacts);
>> void learn_mask(const struct ofpact_learn *, struct flow_wildcards *);
>> diff --git a/lib/meta-flow.c b/lib/meta-flow.c
>> index 40704e6..9d635a3 100644
>> --- a/lib/meta-flow.c
>> +++ b/lib/meta-flow.c
>> @@ -379,10 +379,12 @@ mf_is_mask_valid(const struct mf_field *mf, const
>> union mf_value *mask)
>> }
>>
>> /* Returns true if 'flow' meets the prerequisites for 'mf', false otherwise.
>> + * If a non-NULL 'mask' is passed, zero-valued matches can also be verified.
>> * Sets inspected bits in 'wc', if non-NULL. */
>> -bool
>> -mf_are_prereqs_ok(const struct mf_field *mf, const struct flow *flow,
>> - struct flow_wildcards *wc)
>> +static bool
>> +mf_are_prereqs_ok__(const struct mf_field *mf, const struct flow *flow,
>> + const struct flow_wildcards *mask OVS_UNUSED,
>> + struct flow_wildcards *wc)
>> {
>> switch (mf->prereqs) {
>> case MFP_NONE:
>> @@ -401,6 +403,7 @@ mf_are_prereqs_ok(const struct mf_field *mf, const
>> struct flow *flow,
>> case MFP_IP_ANY:
>> return is_ip_any(flow);
>> case MFP_TCP:
>> + /* Matching !FRAG_LATER is not enforced (mask is not checked). */
>> return is_tcp(flow, wc) && !(flow->nw_frag & FLOW_NW_FRAG_LATER);
>> case MFP_UDP:
>> return is_udp(flow, wc) && !(flow->nw_frag & FLOW_NW_FRAG_LATER);
>> @@ -421,6 +424,23 @@ mf_are_prereqs_ok(const struct mf_field *mf, const
>> struct flow *flow,
>> OVS_NOT_REACHED();
>> }
>>
>> +/* Returns true if 'flow' meets the prerequisites for 'mf', false otherwise.
>> + * Sets inspected bits in 'wc', if non-NULL. */
>> +bool
>> +mf_are_prereqs_ok(const struct mf_field *mf, const struct flow *flow,
>> + struct flow_wildcards *wc)
>> +{
>> + return mf_are_prereqs_ok__(mf, flow, NULL, wc);
>> +}
>> +
>> +/* Returns true if 'match' meets the prerequisites for 'mf', false
>> otherwise.
>> + */
>> +bool
>> +mf_are_match_prereqs_ok(const struct mf_field *mf, const struct match
>> *match)
>> +{
>> + return mf_are_prereqs_ok__(mf, &match->flow, &match->wc, NULL);
>> +}
>> +
>> /* Returns true if 'value' may be a valid value *as part of a masked match*,
>> * false otherwise.
>> *
>> @@ -1948,7 +1968,7 @@ mf_set(const struct mf_field *mf,
>> }
>>
>> static enum ofperr
>> -mf_check__(const struct mf_subfield *sf, const struct flow *flow,
>> +mf_check__(const struct mf_subfield *sf, const struct match *match,
>> const char *type)
>> {
>> if (!sf->field) {
>> @@ -1966,7 +1986,7 @@ mf_check__(const struct mf_subfield *sf, const struct
>> flow *flow,
>> "of %s field %s", sf->ofs, sf->n_bits,
>> sf->field->n_bits, type, sf->field->name);
>> return OFPERR_OFPBAC_BAD_SET_LEN;
>> - } else if (flow && !mf_are_prereqs_ok(sf->field, flow, NULL)) {
>> + } else if (match && !mf_are_match_prereqs_ok(sf->field, match)) {
>> VLOG_WARN_RL(&rl, "%s field %s lacks correct prerequisites",
>> type, sf->field->name);
>> return OFPERR_OFPBAC_MATCH_INCONSISTENT;
>> @@ -2053,18 +2073,18 @@ mf_subfield_swap(const struct mf_subfield *a,
>> * 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, const struct match *match)
>> {
>> - return mf_check__(sf, flow, "source");
>> + return mf_check__(sf, match, "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, const struct match *match)
>> {
>> - int error = mf_check__(sf, flow, "destination");
>> + int error = mf_check__(sf, match, "destination");
>> if (!error && !sf->field->writable) {
>> VLOG_WARN_RL(&rl, "destination field %s is not writable",
>> sf->field->name);
>> diff --git a/lib/multipath.c b/lib/multipath.c
>> index 8a1d1fa..e855a88 100644
>> --- a/lib/multipath.c
>> +++ b/lib/multipath.c
>> @@ -35,9 +35,9 @@
>> * OFPERR_*. */
>> enum ofperr
>> multipath_check(const struct ofpact_multipath *mp,
>> - const struct flow *flow)
>> + const struct match *match)
>> {
>> - return mf_check_dst(&mp->dst, flow);
>> + return mf_check_dst(&mp->dst, match);
>> }
>>
>> /* multipath_execute(). */
>> diff --git a/lib/multipath.h b/lib/multipath.h
>> index 6dd3785..4158302 100644
>> --- a/lib/multipath.h
>> +++ b/lib/multipath.h
>> @@ -24,6 +24,7 @@
>> struct ds;
>> struct flow;
>> struct flow_wildcards;
>> +struct match;
>> struct nx_action_multipath;
>> struct ofpact_multipath;
>> struct ofpbuf;
>> @@ -31,7 +32,7 @@ struct ofpbuf;
>> /* NXAST_MULTIPATH helper functions. */
>>
>> enum ofperr multipath_check(const struct ofpact_multipath *,
>> - const struct flow *);
>> + const struct match *);
>>
>> void multipath_execute(const struct ofpact_multipath *, struct flow *,
>> struct flow_wildcards *);
>> diff --git a/lib/nx-match.c b/lib/nx-match.c
>> index 91401e2..95516a1 100644
>> --- a/lib/nx-match.c
>> +++ b/lib/nx-match.c
>> @@ -539,7 +539,7 @@ nx_pull_raw(const uint8_t *p, unsigned int match_len,
>> bool strict,
>> *cookie = value.be64;
>> *cookie_mask = mask.be64;
>> }
>> - } else if (!mf_are_prereqs_ok(field, &match->flow, NULL)) {
>> + } else if (!mf_are_match_prereqs_ok(field, match)) {
>> error = OFPERR_OFPBMC_BAD_PREREQ;
>> } else if (!mf_is_all_wild(field, &match->wc)) {
>> error = OFPERR_OFPBMC_DUP_FIELD;
>> @@ -1663,16 +1663,17 @@ nxm_format_reg_move(const struct ofpact_reg_move
>> *move, struct ds *s)
>>
>>
>> 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,
>> + const struct match *match)
>> {
>> enum ofperr error;
>>
>> - error = mf_check_src(&move->src, flow);
>> + error = mf_check_src(&move->src, match);
>> if (error) {
>> return error;
>> }
>>
>> - return mf_check_dst(&move->dst, flow);
>> + return mf_check_dst(&move->dst, match);
>> }
>>
>> /* nxm_execute_reg_move(). */
>> @@ -1734,16 +1735,16 @@ nxm_format_stack_pop(const struct ofpact_stack *pop,
>> struct ds *s)
>>
>> enum ofperr
>> nxm_stack_push_check(const struct ofpact_stack *push,
>> - const struct flow *flow)
>> + const struct match *match)
>> {
>> - return mf_check_src(&push->subfield, flow);
>> + return mf_check_src(&push->subfield, match);
>> }
>>
>> enum ofperr
>> nxm_stack_pop_check(const struct ofpact_stack *pop,
>> - const struct flow *flow)
>> + const struct match *match)
>> {
>> - return mf_check_dst(&pop->subfield, flow);
>> + return mf_check_dst(&pop->subfield, match);
>> }
>>
>> /* nxm_execute_stack_push(), nxm_execute_stack_pop().
>> diff --git a/lib/nx-match.h b/lib/nx-match.h
>> index 5dca24a..631ab48 100644
>> --- a/lib/nx-match.h
>> +++ b/lib/nx-match.h
>> @@ -116,7 +116,7 @@ char *nxm_parse_reg_move(struct ofpact_reg_move *, const
>> char *)
>> void nxm_format_reg_move(const struct ofpact_reg_move *, struct ds *);
>>
>> enum ofperr nxm_reg_move_check(const struct ofpact_reg_move *,
>> - const struct flow *);
>> + const struct match *);
>>
>> void nxm_reg_load(const struct mf_subfield *, uint64_t src_data,
>> struct flow *, struct flow_wildcards *);
>> @@ -128,9 +128,9 @@ void nxm_format_stack_push(const struct ofpact_stack *,
>> struct ds *);
>> void nxm_format_stack_pop(const struct ofpact_stack *, struct ds *);
>>
>> enum ofperr nxm_stack_push_check(const struct ofpact_stack *,
>> - const struct flow *);
>> + const struct match *);
>> enum ofperr nxm_stack_pop_check(const struct ofpact_stack *,
>> - const struct flow *);
>> + const struct match *);
>> void nx_stack_push(struct ofpbuf *stack, const void *v, uint8_t bytes);
>> void nx_stack_push_bottom(struct ofpbuf *stack, const void *v, uint8_t
>> bytes);
>> void *nx_stack_pop(struct ofpbuf *stack, uint8_t *bytes);
>> diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
>> index ce80f57..2869e0f 100644
>> --- a/lib/ofp-actions.c
>> +++ b/lib/ofp-actions.c
>> @@ -7065,9 +7065,10 @@ inconsistent_match(enum ofputil_protocol
>> *usable_protocols)
>> * without context. */
>> static enum ofperr
>> ofpact_check__(enum ofputil_protocol *usable_protocols, struct ofpact *a,
>> - struct flow *flow, ofp_port_t max_ports,
>> + struct match *match, ofp_port_t max_ports,
>> uint8_t table_id, uint8_t n_tables)
>> {
>> + struct flow *flow = &match->flow;
>> const struct ofpact_enqueue *enqueue;
>> const struct mf_field *mf;
>>
>> @@ -7089,14 +7090,14 @@ ofpact_check__(enum ofputil_protocol
>> *usable_protocols, struct ofpact *a,
>> 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, match);
>>
>> case OFPACT_OUTPUT_TRUNC:
>> return ofpact_check_output_port(ofpact_get_OUTPUT_TRUNC(a)->port,
>> max_ports);
>>
>> case OFPACT_BUNDLE:
>> - return bundle_check(ofpact_get_BUNDLE(a), max_ports, flow);
>> + return bundle_check(ofpact_get_BUNDLE(a), max_ports, match);
>>
>> case OFPACT_SET_VLAN_VID:
>> /* Remember if we saw a vlan tag in the flow to aid translating to
>> @@ -7179,7 +7180,7 @@ ofpact_check__(enum ofputil_protocol
>> *usable_protocols, struct ofpact *a,
>> 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), match);
>>
>> case OFPACT_SET_FIELD:
>> mf = ofpact_get_SET_FIELD(a)->field;
>> @@ -7202,10 +7203,10 @@ ofpact_check__(enum ofputil_protocol
>> *usable_protocols, struct ofpact *a,
>> return 0;
>>
>> case OFPACT_STACK_PUSH:
>> - return nxm_stack_push_check(ofpact_get_STACK_PUSH(a), flow);
>> + return nxm_stack_push_check(ofpact_get_STACK_PUSH(a), match);
>>
>> case OFPACT_STACK_POP:
>> - return nxm_stack_pop_check(ofpact_get_STACK_POP(a), flow);
>> + return nxm_stack_pop_check(ofpact_get_STACK_POP(a), match);
>>
>> case OFPACT_SET_MPLS_LABEL:
>> case OFPACT_SET_MPLS_TC:
>> @@ -7229,13 +7230,13 @@ ofpact_check__(enum ofputil_protocol
>> *usable_protocols, struct ofpact *a,
>> return 0;
>>
>> case OFPACT_LEARN:
>> - return learn_check(ofpact_get_LEARN(a), flow);
>> + return learn_check(ofpact_get_LEARN(a), match);
>>
>> case OFPACT_CONJUNCTION:
>> return 0;
>>
>> case OFPACT_MULTIPATH:
>> - return multipath_check(ofpact_get_MULTIPATH(a), flow);
>> + return multipath_check(ofpact_get_MULTIPATH(a), match);
>>
>> case OFPACT_NOTE:
>> case OFPACT_EXIT:
>> @@ -7262,7 +7263,7 @@ ofpact_check__(enum ofputil_protocol
>> *usable_protocols, struct ofpact *a,
>> case OFPACT_CLONE: {
>> struct ofpact_nest *on = ofpact_get_CLONE(a);
>> return ofpacts_check(on->actions, ofpact_nest_get_action_len(on),
>> - flow, max_ports, table_id, n_tables,
>> + match, max_ports, table_id, n_tables,
>> usable_protocols);
>> }
>>
>> @@ -7280,11 +7281,11 @@ ofpact_check__(enum ofputil_protocol
>> *usable_protocols, struct ofpact *a,
>> }
>>
>> if (oc->zone_src.field) {
>> - return mf_check_src(&oc->zone_src, flow);
>> + return mf_check_src(&oc->zone_src, match);
>> }
>>
>> return ofpacts_check(oc->actions, ofpact_ct_get_action_len(oc),
>> - flow, max_ports, table_id, n_tables,
>> + match, max_ports, table_id, n_tables,
>> usable_protocols);
>> }
>>
>> @@ -7312,7 +7313,7 @@ ofpact_check__(enum ofputil_protocol
>> *usable_protocols, struct ofpact *a,
>> struct ofpact_nest *on = ofpact_get_WRITE_ACTIONS(a);
>> enum ofputil_protocol p = *usable_protocols;
>> return ofpacts_check(on->actions, ofpact_nest_get_action_len(on),
>> - flow, max_ports, table_id, n_tables, &p);
>> + match, max_ports, table_id, n_tables, &p);
>> }
>>
>> case OFPACT_WRITE_METADATA:
>> @@ -7360,32 +7361,33 @@ ofpact_check__(enum ofputil_protocol
>> *usable_protocols, struct ofpact *a,
>> * example of an inconsistency between match and actions is a flow that does
>> * not match on an MPLS Ethertype but has an action that pops an MPLS label.)
>> *
>> - * May annotate ofpacts with information gathered from the 'flow'.
>> + * May annotate ofpacts with information gathered from the 'match'.
>> *
>> - * May temporarily modify 'flow', but restores the changes before
>> returning. */
>> + * May temporarily modify 'match', but restores the changes before
>> + * returning. */
>> enum ofperr
>> ofpacts_check(struct ofpact ofpacts[], size_t ofpacts_len,
>> - struct flow *flow, ofp_port_t max_ports,
>> + struct match *match, ofp_port_t max_ports,
>> uint8_t table_id, uint8_t n_tables,
>> enum ofputil_protocol *usable_protocols)
>> {
>> struct ofpact *a;
>> - ovs_be16 dl_type = flow->dl_type;
>> - ovs_be16 vlan_tci = flow->vlan_tci;
>> - uint8_t nw_proto = flow->nw_proto;
>> + ovs_be16 dl_type = match->flow.dl_type;
>> + ovs_be16 vlan_tci = match->flow.vlan_tci;
>> + uint8_t nw_proto = match->flow.nw_proto;
>> enum ofperr error = 0;
>>
>> OFPACT_FOR_EACH (a, ofpacts, ofpacts_len) {
>> - error = ofpact_check__(usable_protocols, a, flow,
>> + error = ofpact_check__(usable_protocols, a, match,
>> max_ports, table_id, n_tables);
>> if (error) {
>> break;
>> }
>> }
>> /* Restore fields that may have been modified. */
>> - flow->dl_type = dl_type;
>> - flow->vlan_tci = vlan_tci;
>> - flow->nw_proto = nw_proto;
>> + match->flow.dl_type = dl_type;
>> + match->flow.vlan_tci = vlan_tci;
>> + match->flow.nw_proto = nw_proto;
>> return error;
>> }
>>
>> @@ -7393,14 +7395,14 @@ ofpacts_check(struct ofpact ofpacts[], size_t
>> ofpacts_len,
>> * OFPERR_OFPBAC_MATCH_INCONSISTENT rather than clearing bits. */
>> enum ofperr
>> ofpacts_check_consistency(struct ofpact ofpacts[], size_t ofpacts_len,
>> - struct flow *flow, ofp_port_t max_ports,
>> + struct match *match, ofp_port_t max_ports,
>> uint8_t table_id, uint8_t n_tables,
>> enum ofputil_protocol usable_protocols)
>> {
>> enum ofputil_protocol p = usable_protocols;
>> enum ofperr error;
>>
>> - error = ofpacts_check(ofpacts, ofpacts_len, flow, max_ports,
>> + error = ofpacts_check(ofpacts, ofpacts_len, match, max_ports,
>> table_id, n_tables, &p);
>> return (error ? error
>> : p != usable_protocols ? OFPERR_OFPBAC_MATCH_INCONSISTENT
>> diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
>> index a6b9fcf..7826bc5 100644
>> --- a/lib/ofp-parse.c
>> +++ b/lib/ofp-parse.c
>> @@ -546,7 +546,7 @@ parse_ofp_str__(struct ofputil_flow_mod *fm, int
>> command, char *string,
>> if (!error) {
>> enum ofperr err;
>>
>> - err = ofpacts_check(ofpacts.data, ofpacts.size, &fm->match.flow,
>> + err = ofpacts_check(ofpacts.data, ofpacts.size, &fm->match,
>> OFPP_MAX, fm->table_id, 255,
>> usable_protocols);
>> if (!err && !*usable_protocols) {
>> err = OFPERR_OFPBAC_MATCH_INCONSISTENT;
>> diff --git a/lib/ofp-util.c b/lib/ofp-util.c
>> index 0c9343e..7d40cbb 100644
>> --- a/lib/ofp-util.c
>> +++ b/lib/ofp-util.c
>> @@ -1751,7 +1751,7 @@ ofputil_decode_flow_mod(struct ofputil_flow_mod *fm,
>> }
>>
>> return ofpacts_check_consistency(fm->ofpacts, fm->ofpacts_len,
>> - &fm->match.flow, max_port,
>> + &fm->match, max_port,
>> fm->table_id, max_table, protocol);
>> }
>>
>> diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c
>> index b01a131..a5cc089 100644
>> --- a/ofproto/ofproto-dpif-trace.c
>> +++ b/ofproto/ofproto-dpif-trace.c
>> @@ -324,7 +324,7 @@ ofproto_unixctl_trace_actions(struct unixctl_conn *conn,
>> int argc,
>> struct ofpbuf ofpacts;
>> struct dp_packet *packet;
>> struct ds result;
>> - struct flow flow;
>> + struct match match;
>> uint16_t in_port;
>>
>> /* Three kinds of error return values! */
>> @@ -354,12 +354,13 @@ ofproto_unixctl_trace_actions(struct unixctl_conn
>> *conn, int argc,
>> enforce_consistency = false;
>> }
>>
>> - error = parse_flow_and_packet(argc, argv, &ofproto, &flow, &packet);
>> + error = parse_flow_and_packet(argc, argv, &ofproto, &match.flow,
>> &packet);
>> if (error) {
>> unixctl_command_reply_error(conn, error);
>> free(error);
>> goto exit;
>> }
>> + match_wc_init(&match, &match.flow);
>
> Inside match_wc_init(), this ends up copying the entire flow from the
> current memory into itself - should we avoid that?
>
I don’t this this matters for the trace, but we could change the match_init_*
functions skipping copies between identical addresses?
Jarno
>>
>> /* Do the same checks as handle_packet_out() in ofproto.c.
>> *
>> @@ -371,18 +372,18 @@ ofproto_unixctl_trace_actions(struct unixctl_conn
>> *conn, int argc,
>> *
>> * We skip the "meter" check here because meter is an instruction, not an
>> * action, and thus cannot appear in ofpacts. */
>> - in_port = ofp_to_u16(flow.in_port.ofp_port);
>> + in_port = ofp_to_u16(match.flow.in_port.ofp_port);
>> if (in_port >= ofproto->up.max_ports && in_port < ofp_to_u16(OFPP_MAX)) {
>> unixctl_command_reply_error(conn, "invalid in_port");
>> goto exit;
>> }
>> if (enforce_consistency) {
>> - retval = ofpacts_check_consistency(ofpacts.data, ofpacts.size,
>> &flow,
>> + retval = ofpacts_check_consistency(ofpacts.data, ofpacts.size,
>> &match,
>> u16_to_ofp(ofproto->up.max_ports),
>> 0, ofproto->up.n_tables,
>> usable_protocols);
>> } else {
>> - retval = ofpacts_check(ofpacts.data, ofpacts.size, &flow,
>> + retval = ofpacts_check(ofpacts.data, ofpacts.size, &match,
>> u16_to_ofp(ofproto->up.max_ports), 0,
>> ofproto->up.n_tables, &usable_protocols);
>> }
>> @@ -398,7 +399,7 @@ ofproto_unixctl_trace_actions(struct unixctl_conn *conn,
>> int argc,
>> goto exit;
>> }
>>
>> - ofproto_trace(ofproto, &flow, packet,
>> + ofproto_trace(ofproto, &match.flow, packet,
>> ofpacts.data, ofpacts.size, &result);
>> unixctl_command_reply(conn, ds_cstr(&result));
>>
>> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
>> index ef4b1d9..35ec31d 100644
>> --- a/ofproto/ofproto.c
>> +++ b/ofproto/ofproto.c
>> @@ -3447,6 +3447,7 @@ ofproto_packet_out_init(struct ofproto *ofproto,
>> const struct ofputil_packet_out *po)
>> {
>> enum ofperr error;
>> + struct match match;
>>
>> if (ofp_to_u16(po->in_port) >= ofproto->max_ports
>> && ofp_to_u16(po->in_port) < ofp_to_u16(OFPP_MAX)) {
>> @@ -3471,8 +3472,8 @@ ofproto_packet_out_init(struct ofproto *ofproto,
>> * actions aren't in any table. This is OK as 'table_id' is only used to
>> * check instructions (e.g., goto-table), which can't appear on the
>> action
>> * list of a packet-out. */
>> - error = ofpacts_check_consistency(po->ofpacts, po->ofpacts_len,
>> - opo->flow,
>> + match_wc_init(&match, opo->flow);
>> + error = ofpacts_check_consistency(po->ofpacts, po->ofpacts_len, &match,
>> u16_to_ofp(ofproto->max_ports), 0,
>> ofproto->n_tables,
>> ofconn_get_protocol(ofconn));
>> diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
>> index 77332e0..e317242 100644
>> --- a/utilities/ovs-ofctl.c
>> +++ b/utilities/ovs-ofctl.c
>> @@ -3880,12 +3880,12 @@ ofctl_parse_actions__(const char *version_s, bool
>> instructions)
>> if (!error && instructions) {
>> /* Verify actions, enforce consistency. */
>> enum ofputil_protocol protocol;
>> - struct flow flow;
>> + struct match match;
>>
>> - memset(&flow, 0, sizeof flow);
>> + memset(&match, 0, sizeof match);
>> protocol = ofputil_protocols_from_ofp_version(version);
>> error = ofpacts_check_consistency(ofpacts.data, ofpacts.size,
>> - &flow, OFPP_MAX,
>> + &match, OFPP_MAX,
>> table_id ? atoi(table_id) : 0,
>> OFPTT_MAX + 1, protocol);
>> }
>> --
>> 2.1.4
>>
>> _______________________________________________
>> dev mailing list
>> [email protected] <mailto:[email protected]>
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>> <https://mail.openvswitch.org/mailman/listinfo/ovs-dev>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
