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

[Ding Tianhong <dingtianhong@huawei.com>]

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