Message ID | 1529330677-15328-1-git-send-email-liuhangbin@gmail.com |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
Series | [net] ipvlan: call dev_change_flags when reset ipvlan mode | expand |
On Mon, Jun 18, 2018 at 7:04 AM, Hangbin Liu <liuhangbin@gmail.com> wrote: > @@ -94,10 +95,13 @@ static int ipvlan_set_port_mode(struct ipvl_port *port, u16 nval) > mdev->l3mdev_ops = NULL; > } > list_for_each_entry(ipvlan, &port->ipvlans, pnode) { > + flags = ipvlan->dev->flags; > if (nval == IPVLAN_MODE_L3 || nval == IPVLAN_MODE_L3S) > - ipvlan->dev->flags |= IFF_NOARP; > + dev_change_flags(ipvlan->dev, > + flags | IFF_NOARP); > else > - ipvlan->dev->flags &= ~IFF_NOARP; > + dev_change_flags(ipvlan->dev, > + flags & ~IFF_NOARP); You need to check the return value of dev_change_flags().
On Tue, Jun 19, 2018 at 02:10:18PM -0700, Cong Wang wrote: > On Mon, Jun 18, 2018 at 7:04 AM, Hangbin Liu <liuhangbin@gmail.com> wrote: > > @@ -94,10 +95,13 @@ static int ipvlan_set_port_mode(struct ipvl_port *port, u16 nval) > > mdev->l3mdev_ops = NULL; > > } > > list_for_each_entry(ipvlan, &port->ipvlans, pnode) { > > + flags = ipvlan->dev->flags; > > if (nval == IPVLAN_MODE_L3 || nval == IPVLAN_MODE_L3S) > > - ipvlan->dev->flags |= IFF_NOARP; > > + dev_change_flags(ipvlan->dev, > > + flags | IFF_NOARP); > > else > > - ipvlan->dev->flags &= ~IFF_NOARP; > > + dev_change_flags(ipvlan->dev, > > + flags & ~IFF_NOARP); > > You need to check the return value of dev_change_flags(). Hi Wang Cong, 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? Thanks Hangbin
From: Hangbin Liu <liuhangbin@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.
On Tue, Jun 19, 2018 at 10:31 PM, David Miller <davem@davemloft.net> wrote: > From: Hangbin Liu <liuhangbin@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.
On Wed, Jun 20, 2018 at 10:45:39AM -0700, Cong Wang wrote: > On Tue, Jun 19, 2018 at 10:31 PM, David Miller <davem@davemloft.net> wrote: > > From: Hangbin Liu <liuhangbin@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
diff --git a/drivers/net/ipvlan/ipvlan_main.c b/drivers/net/ipvlan/ipvlan_main.c index 4377c26..368712b 100644 --- a/drivers/net/ipvlan/ipvlan_main.c +++ b/drivers/net/ipvlan/ipvlan_main.c @@ -75,6 +75,7 @@ static int ipvlan_set_port_mode(struct ipvl_port *port, u16 nval) { struct ipvl_dev *ipvlan; struct net_device *mdev = port->dev; + unsigned int flags; int err = 0; ASSERT_RTNL(); @@ -94,10 +95,13 @@ static int ipvlan_set_port_mode(struct ipvl_port *port, u16 nval) mdev->l3mdev_ops = NULL; } list_for_each_entry(ipvlan, &port->ipvlans, pnode) { + flags = ipvlan->dev->flags; if (nval == IPVLAN_MODE_L3 || nval == IPVLAN_MODE_L3S) - ipvlan->dev->flags |= IFF_NOARP; + dev_change_flags(ipvlan->dev, + flags | IFF_NOARP); else - ipvlan->dev->flags &= ~IFF_NOARP; + dev_change_flags(ipvlan->dev, + flags & ~IFF_NOARP); } port->mode = nval; }