From mboxrd@z Thu Jan  1 00:00:00 1970
From: Alex Kiselev <alex@therouter.net>
Subject: Re: [PATCH v3] net/bonding: add add/remove mac addrs
Date: Tue, 19 Jun 2018 12:19:42 +0300
Message-ID: <162811227.20180619121942@therouter.net>
References: <0d1c4f28-e08b-40e7-9b9e-0d4bdd15279a@VE1EUR03FT041.eop-EUR03.prod.protection.outlook.com>
 <VI1PR0501MB2608DAEB263266A3CE7322A6D2700@VI1PR0501MB2608.eurprd05.prod.outlook.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
To: Matan Azrad <matan@mellanox.com>, "dev@dpdk.org" <dev@dpdk.org>,
 Chas   Williams <3chas3@gmail.com>,
 Stephen Hemminger <stephen@networkplumber.org>
Return-path: <dev-bounces@dpdk.org>
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 <dev@dpdk.org>; Tue, 19 Jun 2018 11:19:50 +0200 (CEST)
In-Reply-To: <VI1PR0501MB2608DAEB263266A3CE7322A6D2700@VI1PR0501MB2608.eurprd05.prod.outlook.com>
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

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