On Wed, Jun 20, 2018 at 10:45:39AM -0700, Cong Wang wrote:
> On Tue, Jun 19, 2018 at 10:31 PM, David Miller <da...@davemloft.net> wrote:
> > From: Hangbin Liu <liuhang...@gmail.com>
> > Date: Wed, 20 Jun 2018 11:22:54 +0800
> >
> >> The only case dev_change_flags() return an err is when we change IFF_UP
> >> flag.
> >> Since we only set/reset IFF_NOARP, do you think we still need to check the
> >> return value?
> >
> > It is bad to try and take shortcuts on error handling using assumptions
> > like that.
> >
> > If dev_change_flags() is adjusted to return error codes in more
> > situations, nobody is going to remember to undo your "optimziation"
> > here.
> >
> > Please check for errors, thank you.
>
> Yeah. Also since the notifier is triggered in this case:
>
> if (dev->flags & IFF_UP &&
> (changes & ~(IFF_UP | IFF_PROMISC | IFF_ALLMULTI |
> IFF_VOLATILE))) {
> struct netdev_notifier_change_info change_info = {
> .info = {
> .dev = dev,
> },
> .flags_changed = changes,
> };
>
> call_netdevice_notifiers_info(NETDEV_CHANGE,
> &change_info.info);
> }
>
> the return value of call_netdevice_notifiers_info() isn't captured
> either, but it should be.
Thanks for the explanation. I will fix it.
Regards
Hangbin
