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

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

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?

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