| Message ID | 7c788b65786582209a3ba4bc4d4ed279f6f77568.1458134414.git.lucien.xin@gmail.com |
|---|---|
| State | Deferred, archived |
| Delegated to: | David Miller |
| Headers | show |
On 03/16/2016 02:34 PM, Xin Long wrote: > Now when we change the attributes of bridge or br_port by netlink, > a relevant netlink notification will be sent, but if we change them > by ioctl or sysfs, no notification will be sent. > > We should ensure that whenever those attributes change internally or from > sysfs/ioctl, that a netlink notification is sent out to listeners. > > Also, NetworkManager will use this in the future to listen for out-of-band > bridge master attribute updates and incorporate them into the runtime > configuration. > > This patch is used for br_sysfs_if, and we also move br_ifinfo_notify out > of store_flag. > > Signed-off-by: Xin Long <lucien.xin@gmail.com> > --- > net/bridge/br_sysfs_if.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > Generally looks good, but it creates an inconsistency between bridge fdb_flush and port fdb_flush since the latter will generate a notification while the bridge flush will not.
On Wed, Mar 16, 2016 at 10:23 PM, Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote: > On 03/16/2016 02:34 PM, Xin Long wrote: >> Now when we change the attributes of bridge or br_port by netlink, >> a relevant netlink notification will be sent, but if we change them >> by ioctl or sysfs, no notification will be sent. >> >> We should ensure that whenever those attributes change internally or from >> sysfs/ioctl, that a netlink notification is sent out to listeners. >> >> Also, NetworkManager will use this in the future to listen for out-of-band >> bridge master attribute updates and incorporate them into the runtime >> configuration. >> >> This patch is used for br_sysfs_if, and we also move br_ifinfo_notify out >> of store_flag. >> >> Signed-off-by: Xin Long <lucien.xin@gmail.com> >> --- >> net/bridge/br_sysfs_if.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> > > Generally looks good, but it creates an inconsistency between bridge fdb_flush > and port fdb_flush since the latter will generate a notification while the > bridge flush will not. > yeah, because port fdb_flush is called by brport_store(), in the common function. do you think it''s redundant if we add a notification in bridge fdb_flush to keep consistence with port fdb_flush?
On Wed, Mar 16, 2016 at 10:45 PM, Xin Long <lucien.xin@gmail.com> wrote: > yeah, because port fdb_flush is called by brport_store(), in the > common function. > do you think it''s redundant if we add a notification in bridge > fdb_flush to keep > consistence with port fdb_flush? just change it on patch 1/6.
On 03/16/2016 03:45 PM, Xin Long wrote: > On Wed, Mar 16, 2016 at 10:23 PM, Nikolay Aleksandrov > <nikolay@cumulusnetworks.com> wrote: >> On 03/16/2016 02:34 PM, Xin Long wrote: >>> Now when we change the attributes of bridge or br_port by netlink, >>> a relevant netlink notification will be sent, but if we change them >>> by ioctl or sysfs, no notification will be sent. >>> >>> We should ensure that whenever those attributes change internally or from >>> sysfs/ioctl, that a netlink notification is sent out to listeners. >>> >>> Also, NetworkManager will use this in the future to listen for out-of-band >>> bridge master attribute updates and incorporate them into the runtime >>> configuration. >>> >>> This patch is used for br_sysfs_if, and we also move br_ifinfo_notify out >>> of store_flag. >>> >>> Signed-off-by: Xin Long <lucien.xin@gmail.com> >>> --- >>> net/bridge/br_sysfs_if.c | 5 +++-- >>> 1 file changed, 3 insertions(+), 2 deletions(-) >>> >> >> Generally looks good, but it creates an inconsistency between bridge fdb_flush >> and port fdb_flush since the latter will generate a notification while the >> bridge flush will not. >> > yeah, because port fdb_flush is called by brport_store(), in the > common function. Right. > do you think it''s redundant if we add a notification in bridge > fdb_flush to keep > consistence with port fdb_flush? > Hmm, technically we're doing this via a sysfs option and the netlink fdb flush one will generate a notification, so I'd say let's make them all consistent and make them all generate a notification, and also making the bridge fdb_flush use the bridge_store_parm should be trivial. Thanks, Nik
On Wed, Mar 16, 2016 at 10:49 PM, Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote: > On 03/16/2016 03:45 PM, Xin Long wrote: >> do you think it''s redundant if we add a notification in bridge >> fdb_flush to keep >> consistence with port fdb_flush? >> > Hmm, technically we're doing this via a sysfs option and the netlink fdb flush > one will generate a notification, so I'd say let's make them all consistent and > make them all generate a notification, and also making the bridge fdb_flush use > the bridge_store_parm should be trivial. > okay, I will also make this one use bridge_store_parm. Thanks
diff --git a/net/bridge/br_sysfs_if.c b/net/bridge/br_sysfs_if.c index efe415a..1e04d4d 100644 --- a/net/bridge/br_sysfs_if.c +++ b/net/bridge/br_sysfs_if.c @@ -61,7 +61,6 @@ static int store_flag(struct net_bridge_port *p, unsigned long v, if (flags != p->flags) { p->flags = flags; br_port_flags_change(p, mask); - br_ifinfo_notify(RTM_NEWLINK, p); } return 0; } @@ -253,8 +252,10 @@ static ssize_t brport_store(struct kobject *kobj, spin_lock_bh(&p->br->lock); ret = brport_attr->store(p, val); spin_unlock_bh(&p->br->lock); - if (ret == 0) + if (!ret) { + br_ifinfo_notify(RTM_NEWLINK, p); ret = count; + } } rtnl_unlock(); }
Now when we change the attributes of bridge or br_port by netlink, a relevant netlink notification will be sent, but if we change them by ioctl or sysfs, no notification will be sent. We should ensure that whenever those attributes change internally or from sysfs/ioctl, that a netlink notification is sent out to listeners. Also, NetworkManager will use this in the future to listen for out-of-band bridge master attribute updates and incorporate them into the runtime configuration. This patch is used for br_sysfs_if, and we also move br_ifinfo_notify out of store_flag. Signed-off-by: Xin Long <lucien.xin@gmail.com> --- net/bridge/br_sysfs_if.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)