| Message ID | 1418979417-28867-1-git-send-email-wen.gang.wang@oracle.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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: > -- > 1.8.3.1 > > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 >> 1.8.3.1 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe netdev" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 >> >> > > > . > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 >>> >>> >> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Dec 22, 2014 at 04:30:40PM +0800, Wengang wrote: > OK. Will change as suggested and re-post. Sounds great. Thanks for your work on this. > > 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 > >>> > >>> > >> > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; break; case BOND_SETHWADDR_OLD: case SIOCBONDSETHWADDR:
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(-)