Toshiaki Makita
2017-Oct-27 08:24 UTC
[Bridge] [PATCH net-next v5 2/2] bridge: vlan: signal if anything changed on vlan add
On 2017/10/26 22:41, Nikolay Aleksandrov wrote:> Before this patch there was no way to tell if the vlan add operation > actually changed anything, thus we would always generate a notification > on adds. Let's make the notifications more precise and generate them > only if anything changed, so use the new bool parameter to signal that the > vlan was updated. We cannot return an error because there are valid use > cases that will be broken (e.g. overlapping range add) and also we can't > risk masking errors due to calls into drivers for vlan add which can > potentially return anything. > > Signed-off-by: Nikolay Aleksandrov <nikolay at cumulusnetworks.com>Thanks for the explanation in the previous mail.> ---...> @@ -576,8 +592,11 @@ int br_vlan_add(struct net_bridge *br, u16 vid, u16 flags) > refcount_inc(&vlan->refcnt); > vlan->flags |= BRIDGE_VLAN_INFO_BRENTRY; > vg->num_vlans++; > + *changed = true; > } > - __vlan_add_flags(vlan, flags); > + if (__vlan_add_flags(vlan, flags)) > + *changed = true; > + > return 0; > } > > @@ -601,6 +620,7 @@ int br_vlan_add(struct net_bridge *br, u16 vid, u16 flags) > free_percpu(vlan->stats); > kfree(vlan);We should not set changed=true in this failure case?> } > + *changed = true; > > return ret; > }...> @@ -1050,6 +1083,7 @@ int nbp_vlan_add(struct net_bridge_port *port, u16 vid, u16 flags) > ret = __vlan_add(vlan, flags); > if (ret) > kfree(vlan);Likewise.> + *changed = true; > > return ret; > } >-- Toshiaki Makita
Nikolay Aleksandrov
2017-Oct-27 08:48 UTC
[Bridge] [PATCH net-next v5 2/2] bridge: vlan: signal if anything changed on vlan add
On 27/10/17 11:24, Toshiaki Makita wrote:> On 2017/10/26 22:41, Nikolay Aleksandrov wrote: >> Before this patch there was no way to tell if the vlan add operation >> actually changed anything, thus we would always generate a notification >> on adds. Let's make the notifications more precise and generate them >> only if anything changed, so use the new bool parameter to signal that the >> vlan was updated. We cannot return an error because there are valid use >> cases that will be broken (e.g. overlapping range add) and also we can't >> risk masking errors due to calls into drivers for vlan add which can >> potentially return anything. >> >> Signed-off-by: Nikolay Aleksandrov <nikolay at cumulusnetworks.com> > > Thanks for the explanation in the previous mail. > >> --- > ... >> @@ -576,8 +592,11 @@ int br_vlan_add(struct net_bridge *br, u16 vid, u16 flags) >> refcount_inc(&vlan->refcnt); >> vlan->flags |= BRIDGE_VLAN_INFO_BRENTRY; >> vg->num_vlans++; >> + *changed = true; >> } >> - __vlan_add_flags(vlan, flags); >> + if (__vlan_add_flags(vlan, flags)) >> + *changed = true; >> + >> return 0; >> } >> >> @@ -601,6 +620,7 @@ int br_vlan_add(struct net_bridge *br, u16 vid, u16 flags) >> free_percpu(vlan->stats); >> kfree(vlan); > > We should not set changed=true in this failure case? >Right, I don't know what I was thinking, clearly I missed the fact that we directly clean up and return "ret" with an error. Very good catch, thanks! That should go in the else branch, will send a v6 later with that fixed up. Thanks, Nik>> } >> + *changed = true; >> >> return ret; >> } > ... >> @@ -1050,6 +1083,7 @@ int nbp_vlan_add(struct net_bridge_port *port, u16 vid, u16 flags) >> ret = __vlan_add(vlan, flags); >> if (ret) >> kfree(vlan); > > Likewise. > >> + *changed = true; >> >> return ret; >> } >> >