From patchwork Wed Feb 3 02:13:32 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ben Pfaff X-Patchwork-Id: 577672 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from archives.nicira.com (unknown [IPv6:2600:3c00::f03c:91ff:fe6e:bdf7]) by ozlabs.org (Postfix) with ESMTP id 3C852140556 for ; Wed, 3 Feb 2016 13:13:47 +1100 (AEDT) Received: from archives.nicira.com (localhost [127.0.0.1]) by archives.nicira.com (Postfix) with ESMTP id 649FE106B9; Tue, 2 Feb 2016 18:13:45 -0800 (PST) X-Original-To: dev@openvswitch.org Delivered-To: dev@openvswitch.org Received: from mx3v3.cudamail.com (mx3.cudamail.com [64.34.241.5]) by archives.nicira.com (Postfix) with ESMTPS id D0570102AA for ; Tue, 2 Feb 2016 18:13:44 -0800 (PST) Received: from bar3.cudamail.com (localhost [127.0.0.1]) by mx3v3.cudamail.com (Postfix) with ESMTPS id 160ED1636F4 for ; Tue, 2 Feb 2016 19:13:44 -0700 (MST) X-ASG-Debug-ID: 1454465622-03dd7b0cbe10a5010001-byXFYA Received: from mx1-pf2.cudamail.com ([192.168.24.2]) by bar3.cudamail.com with ESMTP id q2wjyO9EoaZwSV6y (version=TLSv1 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO) for ; Tue, 02 Feb 2016 19:13:42 -0700 (MST) X-Barracuda-Envelope-From: blp@ovn.org X-Barracuda-RBL-Trusted-Forwarder: 192.168.24.2 Received: from unknown (HELO relay5-d.mail.gandi.net) (217.70.183.197) by mx1-pf2.cudamail.com with ESMTPS (DHE-RSA-AES256-SHA encrypted); 3 Feb 2016 02:13:42 -0000 Received-SPF: pass (mx1-pf2.cudamail.com: SPF record at ovn.org designates 217.70.183.197 as permitted sender) X-Barracuda-Apparent-Source-IP: 217.70.183.197 X-Barracuda-RBL-IP: 217.70.183.197 Received: from mfilter15-d.gandi.net (mfilter15-d.gandi.net [217.70.178.143]) by relay5-d.mail.gandi.net (Postfix) with ESMTP id C848A41C074; Wed, 3 Feb 2016 03:13:40 +0100 (CET) X-Virus-Scanned: Debian amavisd-new at mfilter15-d.gandi.net Received: from relay5-d.mail.gandi.net ([IPv6:::ffff:217.70.183.197]) by mfilter15-d.gandi.net (mfilter15-d.gandi.net [::ffff:10.0.15.180]) (amavisd-new, port 10024) with ESMTP id XRQoIWNNdMav; Wed, 3 Feb 2016 03:13:39 +0100 (CET) X-Originating-IP: 208.91.2.3 Received: from sigabrt.benpfaff.org (unknown [208.91.2.3]) (Authenticated sender: blp@ovn.org) by relay5-d.mail.gandi.net (Postfix) with ESMTPSA id 76CA741C051; Wed, 3 Feb 2016 03:13:38 +0100 (CET) X-CudaMail-Envelope-Sender: blp@ovn.org From: Ben Pfaff To: dev@openvswitch.org X-CudaMail-Whitelist-To: dev@openvswitch.org X-CudaMail-MID: CM-E2-201108597 X-CudaMail-DTE: 020216 X-CudaMail-Originating-IP: 217.70.183.197 Date: Tue, 2 Feb 2016 18:13:32 -0800 X-ASG-Orig-Subj: [##CM-E2-201108597##][PATCH] ofproto: Detect and handle errors in ofproto_port_add(). Message-Id: <1454465612-27885-1-git-send-email-blp@ovn.org> X-Mailer: git-send-email 2.1.3 X-Barracuda-Connect: UNKNOWN[192.168.24.2] X-Barracuda-Start-Time: 1454465622 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 Subject: [ovs-dev] [PATCH] ofproto: Detect and handle errors in ofproto_port_add(). 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 update_port() function called in ofproto_port_add() can encounter errors that prevent a port from being added, but nothing was checking for the error and in fact update_port() didn't even pass the error along to its caller. This commit fixes the problem. The scenario that led me to examine this code can be triggered as follows from the sandbox, as long as you change --enable-dummy=override to --enable-dummy=system in ovs-sandbox: ovs-vsctl add-br br0 ovs-vsctl add-port br0 tun0 \ -- set interface tun0 type=stt options:remote_ip=1.2.3.4 ovs-vsctl add-port br0 tun1 \ -- set interface tun1 type=stt options:remote_ip=1.2.3.4 The second add-port will fail due to the duplicate tunnel options, but ofproto_port_add() will not return the error. Instead, it will report to the caller that it succeeded and tell it that it has ofp_port OFPP_NONE (65535), which is invalid and it obviously does not. The result is that you get bizarre log messages like this: tunnel|WARN|tun1: attempting to add tunnel port with same config as port 'tun0' (::->1.2.3.4, key=0, dp port=7471, pkt mark=0) ofproto|WARN|br0: could not add port tun1 (File exists) bridge|INFO|bridge br0: added interface tun1 on port 65535 ofproto|WARN|br0: cannot configure bfd on nonexistent port 65535 ofproto|WARN|br0: cannot configure LLDP on nonexistent port 65535 ofproto|WARN|br0: cannot get STP status on nonexistent port 65535 ofproto|WARN|br0: cannot get RSTP status on nonexistent port 65535 ofproto|WARN|br0: cannot get STP stats on nonexistent port 65535 ofproto|WARN|br0: cannot get STP stats on nonexistent port 65535 VMware-BZ: #1598643 Signed-off-by: Ben Pfaff Acked-by: Justin Pettit --- ofproto/ofproto.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index 3faf42a..bba30ae 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -219,7 +219,7 @@ static void learned_cookies_flush(struct ofproto *, struct ovs_list *dead_cookie static void ofport_destroy__(struct ofport *) OVS_EXCLUDED(ofproto_mutex); static void ofport_destroy(struct ofport *); -static void update_port(struct ofproto *, const char *devname); +static int update_port(struct ofproto *, const char *devname); static int init_ports(struct ofproto *); static void reinit_ports(struct ofproto *); @@ -1962,7 +1962,7 @@ ofproto_port_add(struct ofproto *ofproto, struct netdev *netdev, simap_put(&ofproto->ofp_requests, netdev_name, ofp_to_u16(ofp_port)); - update_port(ofproto, netdev_name); + error = update_port(ofproto, netdev_name); } if (ofp_portp) { *ofp_portp = OFPP_NONE; @@ -2346,7 +2346,7 @@ ofport_equal(const struct ofputil_phy_port *a, /* Adds an ofport to 'p' initialized based on the given 'netdev' and 'opp'. * The caller must ensure that 'p' does not have a conflicting ofport (that is, * one with the same name or port number). */ -static void +static int ofport_install(struct ofproto *p, struct netdev *netdev, const struct ofputil_phy_port *pp) { @@ -2380,7 +2380,7 @@ ofport_install(struct ofproto *p, goto error; } connmgr_send_port_status(p->connmgr, NULL, pp, OFPPR_ADD); - return; + return 0; error: VLOG_WARN_RL(&rl, "%s: could not add port %s (%s)", @@ -2390,6 +2390,7 @@ error: } else { netdev_close(netdev); } + return error; } /* Removes 'ofport' from 'p' and destroys it. */ @@ -2571,13 +2572,14 @@ ofproto_port_get_stats(const struct ofport *port, struct netdev_stats *stats) return error; } -static void +static int update_port(struct ofproto *ofproto, const char *name) { struct ofproto_port ofproto_port; struct ofputil_phy_port pp; struct netdev *netdev; struct ofport *port; + int error = 0; COVERAGE_INC(ofproto_update_port); @@ -2617,13 +2619,15 @@ update_port(struct ofproto *ofproto, const char *name) ofport_remove(port); } ofport_remove_with_name(ofproto, name); - ofport_install(ofproto, netdev, &pp); + error = ofport_install(ofproto, netdev, &pp); } } else { /* Any port named 'name' is gone now. */ ofport_remove_with_name(ofproto, name); } ofproto_port_destroy(&ofproto_port); + + return error; } static int