From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from relay-out4.mail.masterhost.ru (relay-out4.mail.masterhost.ru [83.222.12.14]) by dpdk.org (Postfix) with ESMTP id 6E55D14E8 for ; Tue, 19 Jun 2018 11:19:50 +0200 (CEST) Received: from [37.139.80.50] (helo=nw) by relay4.mail.masterhost.ru with esmtpa envelope from authenticated with alex@therouter.net message id 1fVCnk-0002Ax-8V; Tue, 19 Jun 2018 12:19:48 +0300 Date: Tue, 19 Jun 2018 12:19:42 +0300 From: Alex Kiselev Message-ID: <162811227.20180619121942@therouter.net> To: Matan Azrad , "dev@dpdk.org" , Chas Williams <3chas3@gmail.com>, Stephen Hemminger In-Reply-To: References: <0d1c4f28-e08b-40e7-9b9e-0d4bdd15279a@VE1EUR03FT041.eop-EUR03.prod.protection.outlook.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit X-KLMS-Rule-ID: 1 X-KLMS-Message-Action: clean X-KLMS-AntiSpam-Lua-Profiles: 126000 [Jun 19 2018] X-KLMS-AntiSpam-Version: 5.8.1.0 X-KLMS-AntiSpam-Envelope-From: alex@therouter.net X-KLMS-AntiSpam-Rate: 0 X-KLMS-AntiSpam-Status: not_detected X-KLMS-AntiSpam-Method: none X-KLMS-AntiSpam-Info: LuaCore: 145 145 8115312454756d54ead40e39a46e0295eb9efe85, {rep_avail}, DmarcAF: none X-MS-Exchange-Organization-SCL: -1 X-KLMS-AntiSpam-Interceptor-Info: scan successful X-KLMS-AntiPhishing: not scanned, disabled by settings X-KLMS-AntiVirus: Kaspersky Security for Linux Mail Server, version 8.0.2.16, not scanned, license restriction Subject: Re: [dpdk-dev] [PATCH v3] net/bonding: add add/remove mac addrs X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 19 Jun 2018 09:19:50 -0000 Hi Matan. > Hi Alex > Please see comments below. >> + >> + ret = rte_eth_dev_mac_addr_add(slave_port_id, mac_addr, 0); >> + if (ret < 0) { >> + /* rollback */ >> + for (i--; i > 0; i--) >> + > In case of failure in the first mac address(i=1) you are going to > remove the default mac address(i=0) from the slave. In that case i will be incremented first and will be equal to 0, then for condition will fail and the loop body will not be executed. >> rte_eth_dev_mac_addr_remove(slave_port_id, >> + &bonded_eth_dev->data- >> >mac_addrs[i]); >> + return ret; >> + } >> + } >> + >> + return 0; >> +} >> + >> +/* >> + * Remove additional MAC addresses from the slave */ int >> +slave_remove_mac_addresses(struct rte_eth_dev *bonded_eth_dev, >> + uint16_t slave_port_id) >> +{ >> + int i, ret = 0; >> + struct ether_addr *mac_addr; >> + >> + for (i = 1; i < BOND_MAX_MAC_ADDRS; i++) { >> + mac_addr = &bonded_eth_dev->data->mac_addrs[i]; >> + if (is_same_ether_addr(mac_addr, &null_mac_addr)) >> + break; >> + >> + ret = rte_eth_dev_mac_addr_remove(slave_port_id, >> mac_addr); >> + } > I suggest to return the first error, also in case of all success > with last failure, the code here wrongly returns success. Yeah, you are right. I'll fix it. >> + >> + for (i = 0; i < internals->slave_count; i++) { >> + ret = rte_eth_dev_mac_addr_add(internals->slaves[i].port_id, >> + mac_addr, vmdq); >> + if (ret < 0) { >> + /* rollback */ >> + for (i--; i >= 0; i--) > In case of failure in the first slave(i=0) you are going probably to get memory error (i=-1). The same logic apply here. When i ==-1 the condition will fail and the loop body will not be executed; -- Alex