Re: [PATCH RESEND net-next v2 1/3] bonding: update the primary slave when changing slave's name

[Ding Tianhong <dingtianhong@huawei.com>]

On 2014/1/14 18:51, Veaceslav Falico wrote:
> On Tue, Jan 14, 2014 at 05:08:21PM +0800, Ding Tianhong wrote:
>> If the slave's name changed, and the bond params primary is exist,
>> the bond should deal with the situation in two ways:
>>
>> 1) If the slave was the primary slave yet, clean the primary slave
>>   and reselect active slave.
>> 2) If the slave's new name is as same as bond primary, set the slave
>>   as primary slave and reselect active slave.
>>
>> Thanks for Veaceslav's suggestion.
>>
>> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
>> ---
>> drivers/net/bonding/bond_main.c | 24 ++++++++++++++++++++++--
>> 1 file changed, 22 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index e06c445..64e25d5 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -2860,9 +2860,29 @@ static int bond_slave_netdev_event(unsigned long event,
>>          */
>>         break;
>>     case NETDEV_CHANGENAME:
>> -        /*
>> -         * TODO: handle changing the primary's name
>> +        /* Handle changing the slave's name:
>> +         * 1) If the slave was primary slave,
>> +         * clean the primary slave and reselect
>> +         * active slave.
>> +         * 2) If the slave's new name is same as
>> +         * bond primary, set the slave as primary
>> +         * slave and reselect active slave.
>>          */
>> +        if (slave == bond->primary_slave ||
>> +            !strcmp(bond->params.primary, slave_dev->name)) {
> 
> And if we're in a mode that doesn't use primary, but have the
> params.primary set? Then we'll issue a bond_select_active_slave() in, say,
> 802.3ad mode.
> 
> In the past 24h I've nacked about 5 of your patchsets, with you keeping
> 'quickfixing' them, without getting your time to understand the issues, and
> re-sending them for review.
> 
> I'm not willing to waste my time that uselessly, reviewing patchsets that
> you randomly generate in the hope of getting it right. And given your
> 'good' history - with patchsets that cause regressions and bugs, with
> reverts because of that, with those horrible, meaningless RCU transition
> that is just plainly wrong and *really* hard to fix - I'm going to react as
> Greg KH said in one of his presentations - NAK your patches and make them
> by myself. It will take *a lot* lesser time from my side, and will
> eventually make the code better.
> 
> Thanks for the report, I'll send a patch that fixes it soon.
> 
> Nacked-by: Veaceslav Falico <vfalico@redhat.com>

Maybe I am not in the state all day, always miss here and miss there, sorry for that.

> 
>> +            if (bond->primary_slave) {
>> +                pr_info("%s: Setting primary slave to None.\n",
>> +                    bond->dev->name);
>> +                bond->primary_slave = NULL;
>> +            } else {
>> +                pr_info("%s: Setting %s as primary slave.\n",
>> +                    bond->dev->name, slave_dev->name);
>> +                bond->primary_slave = slave;
>> +            }
>> +            write_lock_bh(&bond->curr_slave_lock);
>> +            bond_select_active_slave(bond);
>> +            write_unlock_bh(&bond->curr_slave_lock);
>> +        }
>>         break;
>>     case NETDEV_FEAT_CHANGE:
>>         bond_compute_features(bond);
>> -- 
>> 1.8.0
>>
>>
> 
> .
>