Re: [PATCH] bonding: avoid re-entry of bond_release

[Wengang <wen.gang.wang@oracle.com>]

OK. Will change as suggested and re-post.

thanks,
wengang

于 2014年12月22日 10:05, Ding Tianhong 写道:
> On 2014/12/22 9:09, Wengang wrote:
>> Hi Andy and Ding,
>>
>> Thanks for your reviews!
>> In the ioctl path, removing a interface that is not currently actually a slave
>> can happen from user space(by mistake), we should avoid the noisy message.
>>
>> While, __bond_release_one() has another call path which is from bond_uninit().
>> In the later case, it should be treated as an error if the interface is not with
>> IFF_SLAVE flag. To notice that error occurred, the message is printed. I think
>> the message is needed for this path.
>>
>> How do you think?
>>
> Just like the bond_enslave(), it is only a warning.
>
> Ding
>
>> thanks,
>> wengang
>>
>> 于 2014年12月21日 10:01, Ding Tianhong 写道:
>>> On 2014/12/19 23:11, Andy Gospodarek wrote:
>>>> On Fri, Dec 19, 2014 at 04:56:57PM +0800, Wengang Wang wrote:
>>>>> If bond_release is run against an interface which is already detached from
>>>>> it's master, then there is an error message shown like
>>>>>      "<master name> cannot release <slave name>".
>>>>>
>>>>> The call path is:
>>>>>      bond_do_ioctl()
>>>>>          bond_release()
>>>>>              __bond_release_one()
>>>>>
>>>>> Though it does not really harm, the message the message is misleading.
>>>>> This patch tries to avoid the message.
>>>>>
>>>>> Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>
>>>>> ---
>>>>>    drivers/net/bonding/bond_main.c | 5 ++++-
>>>>>    1 file changed, 4 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>>>>> index 184c434..4a71bbd 100644
>>>>> --- a/drivers/net/bonding/bond_main.c
>>>>> +++ b/drivers/net/bonding/bond_main.c
>>>>> @@ -3256,7 +3256,10 @@ static int bond_do_ioctl(struct net_device *bond_dev, struct ifreq *ifr, int cmd
>>>>>            break;
>>>>>        case BOND_RELEASE_OLD:
>>>>>        case SIOCBONDRELEASE:
>>>>> -        res = bond_release(bond_dev, slave_dev);
>>>>> +        if (slave_dev->flags & IFF_SLAVE)
>>>>> +            res = bond_release(bond_dev, slave_dev);
>>>>> +        else
>>>>> +            res = 0;
>>>> Functionally this patch is fine, but I would prefer that you simply
>>>> change the check in __bond_release_one to not be so noisy.  There is a
>>>> check[1] in bond_enslave to see if a slave is already in a bond and that
>>>> just prints a message of netdev_dbg (rather than netdev_err) and it
>>>> seems that would be appropriate for this type of message.
>>>>
>>>> [1] from bond_enslave():
>>>>
>>>>           /* already enslaved */
>>>>           if (slave_dev->flags & IFF_SLAVE) {
>>>>                   netdev_dbg(bond_dev, "Error: Device was already enslaved\n");
>>>>                   return -EBUSY;
>>>>           }
>>>>
>>>>
>>>>>            break;
>>>>>        case BOND_SETHWADDR_OLD:
>>>>>        case SIOCBONDSETHWADDR:
>>>>> -- 
>>> agree ,use netdev_dbg looks more better and enough.
>>>
>>> Ding
>>>
>>>
>>