Re: [dpdk-dev] [PATCH v3] net/bonding: add add/remove mac addrs

[Matan Azrad <matan@mellanox.com>]

 From: Alex Kiselev
> 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.
> 

Yes, my mistake.

> 
> >>       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;

Yes, my mistake.

> 
> 
> 
> 
> --
> Alex