From patchwork Tue Sep 29 23:33:39 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ben Pfaff X-Patchwork-Id: 524066 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from archives.nicira.com (li376-54.members.linode.com [96.126.127.54]) by ozlabs.org (Postfix) with ESMTP id A0BCD140180 for ; Wed, 30 Sep 2015 09:33:54 +1000 (AEST) Received: from archives.nicira.com (localhost [127.0.0.1]) by archives.nicira.com (Postfix) with ESMTP id 6CF83109B5; Tue, 29 Sep 2015 16:33:50 -0700 (PDT) X-Original-To: dev@openvswitch.org Delivered-To: dev@openvswitch.org Received: from mx1e3.cudamail.com (mx1.cudamail.com [69.90.118.67]) by archives.nicira.com (Postfix) with ESMTPS id E9690109AA for ; Tue, 29 Sep 2015 16:33:48 -0700 (PDT) Received: from bar5.cudamail.com (localhost [127.0.0.1]) by mx1e3.cudamail.com (Postfix) with ESMTPS id 72F82420098 for ; Tue, 29 Sep 2015 17:33:48 -0600 (MDT) X-ASG-Debug-ID: 1443569627-09eadd11e06ea000001-byXFYA Received: from mx1-pf2.cudamail.com ([192.168.24.2]) by bar5.cudamail.com with ESMTP id GM8fdq8CJyQVweif (version=TLSv1 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO) for ; Tue, 29 Sep 2015 17:33:47 -0600 (MDT) X-Barracuda-Envelope-From: blp@nicira.com X-Barracuda-RBL-Trusted-Forwarder: 192.168.24.2 Received: from unknown (HELO mail-pa0-f46.google.com) (209.85.220.46) by mx1-pf2.cudamail.com with ESMTPS (RC4-SHA encrypted); 29 Sep 2015 23:33:47 -0000 Received-SPF: unknown (mx1-pf2.cudamail.com: Multiple SPF records returned) X-Barracuda-Apparent-Source-IP: 209.85.220.46 X-Barracuda-RBL-IP: 209.85.220.46 Received: by pablk4 with SMTP id lk4so19172397pab.3 for ; Tue, 29 Sep 2015 16:33:47 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=JTJixXRO94fudnZyDQfrNwqkl3M58HB+Phtr0oSBW0k=; b=C8EGnYGsOEbCBZqtLqkuiuyBHRuwcY9KFACXJJgmOXDKP1ri0jgqTbvRYFvnm/LhlT SRMVdV8GZGhn7NyZzQ88234Y0T7djaZJdpwMn7dMx6tFPdlKK/9fl4ywcEFXAM7dgWlJ VbSNtVbIuxmHbBDLJkKEZ98c6HS+2X+Pd7/c6+cCx1RbdSLFKm3vJU0C7I0IgAQ4Oqug c8RG5FXukL5QTdi0IiR7PbzgXPlwtKa694cWVFCMKAowRNLQxELZLA5OQ/Tn5igQztyT KP4JM8PB1FZzbn2iyS+blPi3zVjg6ICRL96Zk9xzMs1Wmz2oq4LER/BBh3QcJQMWN8hg qQXg== X-Gm-Message-State: ALoCoQngxtaCE87SGmoaV1uBb7k4HmG5XTeS2ao26oKhXpabAapFfwp1uAKbg1IAPzl5uH8PuZvD X-Received: by 10.68.143.4 with SMTP id sa4mr783874pbb.111.1443569627076; Tue, 29 Sep 2015 16:33:47 -0700 (PDT) Received: from sigabrt.benpfaff.org ([208.91.2.4]) by smtp.gmail.com with ESMTPSA id ct2sm27644320pbc.34.2015.09.29.16.33.45 (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Tue, 29 Sep 2015 16:33:45 -0700 (PDT) X-CudaMail-Envelope-Sender: blp@nicira.com From: Ben Pfaff To: dev@openvswitch.org X-CudaMail-Whitelist-To: dev@openvswitch.org X-CudaMail-MID: CM-E2-928118621 X-CudaMail-DTE: 092915 X-CudaMail-Originating-IP: 209.85.220.46 Date: Tue, 29 Sep 2015 16:33:39 -0700 X-ASG-Orig-Subj: [##CM-E2-928118621##][PATCH v2 1/3] ofp-actions: Look inside write_actions for output ports and groups. Message-Id: <1443569621-30566-2-git-send-email-blp@nicira.com> X-Mailer: git-send-email 2.1.3 In-Reply-To: <1443569621-30566-1-git-send-email-blp@nicira.com> References: <1443569621-30566-1-git-send-email-blp@nicira.com> X-Barracuda-Connect: UNKNOWN[192.168.24.2] X-Barracuda-Start-Time: 1443569627 X-Barracuda-Encrypted: DHE-RSA-AES256-SHA X-Barracuda-URL: https://web.cudamail.com:443/cgi-mod/mark.cgi X-ASG-Whitelist: Header =?UTF-8?B?eFwtY3VkYW1haWxcLXdoaXRlbGlzdFwtdG8=?= X-Virus-Scanned: by bsmtpd at cudamail.com X-Barracuda-BRTS-Status: 1 Cc: Ben Pfaff , Gavin Remaley Subject: [ovs-dev] [PATCH v2 1/3] ofp-actions: Look inside write_actions for output ports and groups. X-BeenThere: dev@openvswitch.org X-Mailman-Version: 2.1.16 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Errors-To: dev-bounces@openvswitch.org Sender: "dev" 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 --- AUTHORS | 1 + lib/ofp-actions.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++-- lib/ofp-actions.h | 12 ++++++++++ ofproto/ofproto.c | 2 +- tests/ofproto-dpif.at | 5 ++-- 5 files changed, 81 insertions(+), 5 deletions(-) diff --git a/AUTHORS b/AUTHORS index cba0535..168566d 100644 --- a/AUTHORS +++ b/AUTHORS @@ -260,6 +260,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 88f0f85..a4bddbf 100644 --- a/lib/ofp-actions.c +++ b/lib/ofp-actions.c @@ -347,6 +347,68 @@ static char *OVS_WARN_UNUSED_RESULT ofpacts_parse( char *str, struct ofpbuf *ofpacts, 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: + return ofpact_next(ofpact); + + 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. * @@ -6175,7 +6237,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; } @@ -6192,7 +6254,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 51b2963..d0eb1d2 100644 --- a/lib/ofp-actions.h +++ b/lib/ofp-actions.h @@ -183,12 +183,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) { @@ -200,6 +203,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 c7dd8a2..1f37f6c 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 9609d2d..007856e 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at @@ -477,7 +477,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 @@ -498,7 +498,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 @@ -650,6 +650,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