From: Ben Pfaff Date: Sun, 29 Nov 2015 18:51:25 +0000 (-0800) Subject: ofp-actions: Look inside write_actions for output ports and groups. X-Git-Tag: v2.5.0~249 X-Git-Url: http://git.cascardo.eti.br/?a=commitdiff_plain;ds=sidebyside;h=4f20179ddc79843ef89f68d2c8342df0eb69a814;p=cascardo%2Fovs.git ofp-actions: Look inside write_actions for output ports and groups. The out_port and out_group matches only looked at apply_actions instructions, but my interpretation of the OpenFlow spec is that they should also look inside write_actions. This affected the output of (and in one case the correctness of) some tests, so this updates them. Reported-by: Gavin Remaley Signed-off-by: Ben Pfaff Reviewed-by: Simon Horman --- diff --git a/AUTHORS b/AUTHORS index 79a049ec4..e0ca51da9 100644 --- a/AUTHORS +++ b/AUTHORS @@ -266,6 +266,7 @@ Eivind Bulie Haanaes Eric Lopez elopez@nicira.com Frido Roose fr.roose@gmail.com Gaetano Catalli gaetano.catalli@gmail.com +Gavin Remaley gavin_remaley@selinc.com George Shuklin amarao@desunote.ru Gerald Rogers gerald.rogers@intel.com Ghanem Bahri bahri.ghanem@gmail.com diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c index 2a71e7ac7..8050fe401 100644 --- a/lib/ofp-actions.c +++ b/lib/ofp-actions.c @@ -364,6 +364,72 @@ static char * OVS_WARN_UNUSED_RESULT ofpacts_parse_copy( enum ofputil_protocol *usable_protocols, bool allow_instructions, enum ofpact_type outer_action); +/* Returns the ofpact following 'ofpact', except that if 'ofpact' contains + * nested ofpacts it returns the first one. */ +struct ofpact * +ofpact_next_flattened(const struct ofpact *ofpact) +{ + switch (ofpact->type) { + case OFPACT_OUTPUT: + case OFPACT_GROUP: + case OFPACT_CONTROLLER: + case OFPACT_ENQUEUE: + case OFPACT_OUTPUT_REG: + case OFPACT_BUNDLE: + case OFPACT_SET_FIELD: + case OFPACT_SET_VLAN_VID: + case OFPACT_SET_VLAN_PCP: + case OFPACT_STRIP_VLAN: + case OFPACT_PUSH_VLAN: + case OFPACT_SET_ETH_SRC: + case OFPACT_SET_ETH_DST: + case OFPACT_SET_IPV4_SRC: + case OFPACT_SET_IPV4_DST: + case OFPACT_SET_IP_DSCP: + case OFPACT_SET_IP_ECN: + case OFPACT_SET_IP_TTL: + case OFPACT_SET_L4_SRC_PORT: + case OFPACT_SET_L4_DST_PORT: + case OFPACT_REG_MOVE: + case OFPACT_STACK_PUSH: + case OFPACT_STACK_POP: + case OFPACT_DEC_TTL: + case OFPACT_SET_MPLS_LABEL: + case OFPACT_SET_MPLS_TC: + case OFPACT_SET_MPLS_TTL: + case OFPACT_DEC_MPLS_TTL: + case OFPACT_PUSH_MPLS: + case OFPACT_POP_MPLS: + case OFPACT_SET_TUNNEL: + case OFPACT_SET_QUEUE: + case OFPACT_POP_QUEUE: + case OFPACT_FIN_TIMEOUT: + case OFPACT_RESUBMIT: + case OFPACT_LEARN: + case OFPACT_CONJUNCTION: + case OFPACT_MULTIPATH: + case OFPACT_NOTE: + case OFPACT_EXIT: + case OFPACT_SAMPLE: + case OFPACT_UNROLL_XLATE: + case OFPACT_DEBUG_RECIRC: + case OFPACT_METER: + case OFPACT_CLEAR_ACTIONS: + case OFPACT_WRITE_METADATA: + case OFPACT_GOTO_TABLE: + case OFPACT_NAT: + return ofpact_next(ofpact); + + case OFPACT_CT: + return ofpact_get_CT(ofpact)->actions; + + case OFPACT_WRITE_ACTIONS: + return ofpact_get_WRITE_ACTIONS(ofpact)->actions; + } + + OVS_NOT_REACHED(); +} + /* Pull off existing actions or instructions. Used by nesting actions to keep * ofpacts_parse() oblivious of actions nesting. * @@ -7096,7 +7162,7 @@ ofpacts_output_to_port(const struct ofpact *ofpacts, size_t ofpacts_len, { const struct ofpact *a; - OFPACT_FOR_EACH (a, ofpacts, ofpacts_len) { + OFPACT_FOR_EACH_FLATTENED (a, ofpacts, ofpacts_len) { if (ofpact_outputs_to_port(a, port)) { return true; } @@ -7113,7 +7179,7 @@ ofpacts_output_to_group(const struct ofpact *ofpacts, size_t ofpacts_len, { const struct ofpact *a; - OFPACT_FOR_EACH (a, ofpacts, ofpacts_len) { + OFPACT_FOR_EACH_FLATTENED (a, ofpacts, ofpacts_len) { if (a->type == OFPACT_GROUP && ofpact_get_GROUP(a)->group_id == group_id) { return true; diff --git a/lib/ofp-actions.h b/lib/ofp-actions.h index 5e97c88d1..a07fb3f2e 100644 --- a/lib/ofp-actions.h +++ b/lib/ofp-actions.h @@ -185,12 +185,15 @@ BUILD_ASSERT_DECL(sizeof(struct ofpact) == 4); #define OFPACT_ALIGNTO 8 #define OFPACT_ALIGN(SIZE) ROUND_UP(SIZE, OFPACT_ALIGNTO) +/* Returns the ofpact following 'ofpact'. */ static inline struct ofpact * ofpact_next(const struct ofpact *ofpact) { return (void *) ((uint8_t *) ofpact + OFPACT_ALIGN(ofpact->len)); } +struct ofpact *ofpact_next_flattened(const struct ofpact *); + static inline struct ofpact * ofpact_end(const struct ofpact *ofpacts, size_t ofpacts_len) { @@ -202,6 +205,15 @@ ofpact_end(const struct ofpact *ofpacts, size_t ofpacts_len) #define OFPACT_FOR_EACH(POS, OFPACTS, OFPACTS_LEN) \ for ((POS) = (OFPACTS); (POS) < ofpact_end(OFPACTS, OFPACTS_LEN); \ (POS) = ofpact_next(POS)) + +/* Assigns POS to each ofpact, in turn, in the OFPACTS_LEN bytes of ofpacts + * starting at OFPACTS. + * + * For ofpacts that contain nested ofpacts, this visits each of the inner + * ofpacts as well. */ +#define OFPACT_FOR_EACH_FLATTENED(POS, OFPACTS, OFPACTS_LEN) \ + for ((POS) = (OFPACTS); (POS) < ofpact_end(OFPACTS, OFPACTS_LEN); \ + (POS) = ofpact_next_flattened(POS)) /* Action structure for each OFPACT_*. */ diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index e41ea3948..4ab86b16e 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -3345,7 +3345,7 @@ ofproto_check_ofpacts(struct ofproto *ofproto, return OFPERR_OFPMMFC_INVALID_METER; } - OFPACT_FOR_EACH (a, ofpacts, ofpacts_len) { + OFPACT_FOR_EACH_FLATTENED (a, ofpacts, ofpacts_len) { if (a->type == OFPACT_GROUP && !ofproto_group_exists(ofproto, ofpact_get_GROUP(a)->group_id)) { return OFPERR_OFPBAC_BAD_OUT_GROUP; diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index 0af01ca05..60734db42 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at @@ -476,7 +476,7 @@ for i in `seq 0 2`; AT_CHECK([ovs-appctl revalidator/purge], [0]) AT_CHECK([ovs-ofctl -O OpenFlow12 -vwarn dump-group-stats br0], [0], [stdout]) AT_CHECK([STRIP_XIDS stdout | sort], [0], [dnl - group_id=1234,ref_count=0,packet_count=3,byte_count=180,bucket0:packet_count=3,byte_count=180,bucket1:packet_count=0,byte_count=0 + group_id=1234,ref_count=1,packet_count=3,byte_count=180,bucket0:packet_count=3,byte_count=180,bucket1:packet_count=0,byte_count=0 OFPST_GROUP reply (OF1.2): ]) OVS_VSWITCHD_STOP @@ -497,7 +497,7 @@ for i in `seq 0 2`; AT_CHECK([ovs-appctl revalidator/purge], [0]) AT_CHECK([ovs-ofctl -O OpenFlow12 -vwarn dump-group-stats br0], [0], [stdout]) AT_CHECK([STRIP_XIDS stdout | sort], [0], [dnl - group_id=1234,ref_count=0,packet_count=3,byte_count=180,bucket0:packet_count=3,byte_count=180,bucket1:packet_count=3,byte_count=180 + group_id=1234,ref_count=1,packet_count=3,byte_count=180,bucket0:packet_count=3,byte_count=180,bucket1:packet_count=3,byte_count=180 OFPST_GROUP reply (OF1.2): ]) OVS_VSWITCHD_STOP @@ -649,6 +649,7 @@ table=8,actions=clear_actions,write_actions(output(3),output(2)),goto_table(9) table=9,priority=20,actset_output=2 actions=12 table=9,priority=10 actions=13 ]) +AT_CHECK([ovs-ofctl -O OpenFlow13 add-group br0 'group_id=5,type=all,bucket=output:1']) AT_CHECK([ovs-ofctl -O OpenFlow13 add-flows br0 flows.txt]) AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=128,frag=no),icmp(type=8,code=0)'], [0], [stdout]) AT_CHECK([tail -1 stdout], [0], [Datapath actions: 4,6,8,10,12,2