Stephen Hemminger
2018-Jul-20 15:57 UTC
[Bridge] [PATCH net-next 1/2] net: bridge: add support for raw sysfs port options
On Fri, 20 Jul 2018 17:48:25 +0300 Nikolay Aleksandrov <nikolay at cumulusnetworks.com> wrote:> This patch adds a new alternative store callback for port sysfs options > which takes a raw value (buf) and can use it directly. It is needed for the > backup port sysfs support since we have to pass the device by its name. > > Signed-off-by: Nikolay Aleksandrov <nikolay at cumulusnetworks.com> > --- > There are a few checkpatch warnings here because of the old code, I've > noted them in my todo and will send separate patch for simple_strtoul > and the function prototypes. > > net/bridge/br_sysfs_if.c | 49 +++++++++++++++++++++++++++++++++--------------- > 1 file changed, 34 insertions(+), 15 deletions(-) > > diff --git a/net/bridge/br_sysfs_if.c b/net/bridge/br_sysfs_if.c > index f99c5bf5c906..38c58879423d 100644 > --- a/net/bridge/br_sysfs_if.c > +++ b/net/bridge/br_sysfs_if.c > @@ -25,6 +25,15 @@ struct brport_attribute { > struct attribute attr; > ssize_t (*show)(struct net_bridge_port *, char *); > int (*store)(struct net_bridge_port *, unsigned long); > + int (*store_raw)(struct net_bridge_port *, char *); > +}; > + > +#define BRPORT_ATTR_RAW(_name, _mode, _show, _store) \ > +const struct brport_attribute brport_attr_##_name = { \ > + .attr = {.name = __stringify(_name), \ > + .mode = _mode }, \ > + .show = _show, \ > + .store_raw = _store, \ > }; > > #define BRPORT_ATTR(_name, _mode, _show, _store) \ > @@ -270,27 +279,37 @@ static ssize_t brport_store(struct kobject *kobj, > struct brport_attribute *brport_attr = to_brport_attr(attr); > struct net_bridge_port *p = to_brport(kobj); > ssize_t ret = -EINVAL; > - char *endp; > unsigned long val; > + char *endp; > > if (!ns_capable(dev_net(p->dev)->user_ns, CAP_NET_ADMIN)) > return -EPERM; > > - val = simple_strtoul(buf, &endp, 0); > - if (endp != buf) { > - if (!rtnl_trylock()) > - return restart_syscall(); > - if (p->dev && p->br && brport_attr->store) { > - spin_lock_bh(&p->br->lock); > - ret = brport_attr->store(p, val); > - spin_unlock_bh(&p->br->lock); > - if (!ret) { > - br_ifinfo_notify(RTM_NEWLINK, NULL, p); > - ret = count; > - } > - } > - rtnl_unlock(); > + if (!rtnl_trylock()) > + return restart_syscall(); > + > + if (!p->dev || !p->br) > + goto out_unlock; > + > + if (brport_attr->store_raw) { > + spin_lock_bh(&p->br->lock); > + ret = brport_attr->store_raw(p, (char *)buf); > + spin_unlock_bh(&p->br->lock); > + } else if (brport_attr->store) { > + val = simple_strtoul(buf, &endp, 0); > + if (endp == buf) > + goto out_unlock; > + spin_lock_bh(&p->br->lock); > + ret = brport_attr->store(p, val); > + spin_unlock_bh(&p->br->lock); > + } > + if (!ret) { > + br_ifinfo_notify(RTM_NEWLINK, NULL, p); > + ret = count; > } > +out_unlock: > + rtnl_unlock(); > + > return ret; > } >I see where you are going with this. You want a sysfs attribute where is a string not a number. This makes sense. The code might be simpler if you always acquired the lock. Or maybe move the locking into the store functions. Casting away the const on the buf variable is going to cause warnings and should not be necessary. You might also want an error case if neither store or store_raw are defined.
Nikolay Aleksandrov
2018-Jul-20 17:14 UTC
[Bridge] [PATCH net-next 1/2] net: bridge: add support for raw sysfs port options
On July 20, 2018 6:57:25 PM GMT+03:00, Stephen Hemminger <stephen at networkplumber.org> wrote:>On Fri, 20 Jul 2018 17:48:25 +0300 >Nikolay Aleksandrov <nikolay at cumulusnetworks.com> wrote: > >> This patch adds a new alternative store callback for port sysfs >options >> which takes a raw value (buf) and can use it directly. It is needed >for the >> backup port sysfs support since we have to pass the device by its >name. >> >> Signed-off-by: Nikolay Aleksandrov <nikolay at cumulusnetworks.com> >> --- >> There are a few checkpatch warnings here because of the old code, >I've >> noted them in my todo and will send separate patch for simple_strtoul >> and the function prototypes. >> >> net/bridge/br_sysfs_if.c | 49 >+++++++++++++++++++++++++++++++++--------------- >> 1 file changed, 34 insertions(+), 15 deletions(-) >> >> diff --git a/net/bridge/br_sysfs_if.c b/net/bridge/br_sysfs_if.c >> index f99c5bf5c906..38c58879423d 100644 >> --- a/net/bridge/br_sysfs_if.c >> +++ b/net/bridge/br_sysfs_if.c >> @@ -25,6 +25,15 @@ struct brport_attribute { >> struct attribute attr; >> ssize_t (*show)(struct net_bridge_port *, char *); >> int (*store)(struct net_bridge_port *, unsigned long); >> + int (*store_raw)(struct net_bridge_port *, char *); >> +}; >> + >> +#define BRPORT_ATTR_RAW(_name, _mode, _show, _store) \ >> +const struct brport_attribute brport_attr_##_name = { \ >> + .attr = {.name = __stringify(_name), \ >> + .mode = _mode }, \ >> + .show = _show, \ >> + .store_raw = _store, \ >> }; >> >> #define BRPORT_ATTR(_name, _mode, _show, _store) \ >> @@ -270,27 +279,37 @@ static ssize_t brport_store(struct kobject >*kobj, >> struct brport_attribute *brport_attr = to_brport_attr(attr); >> struct net_bridge_port *p = to_brport(kobj); >> ssize_t ret = -EINVAL; >> - char *endp; >> unsigned long val; >> + char *endp; >> >> if (!ns_capable(dev_net(p->dev)->user_ns, CAP_NET_ADMIN)) >> return -EPERM; >> >> - val = simple_strtoul(buf, &endp, 0); >> - if (endp != buf) { >> - if (!rtnl_trylock()) >> - return restart_syscall(); >> - if (p->dev && p->br && brport_attr->store) { >> - spin_lock_bh(&p->br->lock); >> - ret = brport_attr->store(p, val); >> - spin_unlock_bh(&p->br->lock); >> - if (!ret) { >> - br_ifinfo_notify(RTM_NEWLINK, NULL, p); >> - ret = count; >> - } >> - } >> - rtnl_unlock(); >> + if (!rtnl_trylock()) >> + return restart_syscall(); >> + >> + if (!p->dev || !p->br) >> + goto out_unlock; >> + >> + if (brport_attr->store_raw) { >> + spin_lock_bh(&p->br->lock); >> + ret = brport_attr->store_raw(p, (char *)buf); >> + spin_unlock_bh(&p->br->lock); >> + } else if (brport_attr->store) { >> + val = simple_strtoul(buf, &endp, 0); >> + if (endp == buf) >> + goto out_unlock; >> + spin_lock_bh(&p->br->lock); >> + ret = brport_attr->store(p, val); >> + spin_unlock_bh(&p->br->lock); >> + } >> + if (!ret) { >> + br_ifinfo_notify(RTM_NEWLINK, NULL, p); >> + ret = count; >> } >> +out_unlock: >> + rtnl_unlock(); >> + >> return ret; >> } >> > >I see where you are going with this. You want a sysfs attribute where >is a string >not a number. This makes sense. > >The code might be simpler if you always acquired the lock. Or maybe >move the locking into the store functions. >I considered that but wanted to limit the lock because it blocks learning. Since the operations are relatively simple I'll move it earlier.>Casting away the const on the buf variable is going to cause warnings >and should not be necessary. >It doesn't when it's casted like that, the new line is changed to null byte so we need to drop the const.>You might also want an error case if neither store or store_raw are >defined.It is already there, just try writing to any other attribute that doesn't have store.