[Bridge] [PATCH net-next 1/2] net: bridge: add support for raw sysfs port options
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
---
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 attributeattr;
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;
}
--
2.11.0
Re: [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 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
> ---
> 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 attributeattr;
> 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.
Re: [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
wrote:
>On Fri, 20 Jul 2018 17:48:25 +0300
>Nikolay Aleksandrov 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
>> ---
>> 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 attributeattr;
>> 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.
Re: [Bridge] [PATCH net-next 1/2] net: bridge: add support for raw sysfs port options
On Fri, 20 Jul 2018 20:14:43 +0300 Nikolay Aleksandrov wrote: > >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. Then change store function to take a char *?
Re: [Bridge] [PATCH net-next 1/2] net: bridge: add support for raw sysfs port options
On July 20, 2018 8:20:44 PM GMT+03:00, Stephen Hemminger wrote: >On Fri, 20 Jul 2018 20:14:43 +0300 >Nikolay Aleksandrov wrote: > >> >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. > >Then change store function to take a char *? Sure, will do.
Re: [Bridge] [PATCH net-next 1/2] net: bridge: add support for raw sysfs port options
On July 20, 2018 8:26:36 PM GMT+03:00, Nikolay Aleksandrov wrote: >On July 20, 2018 8:20:44 PM GMT+03:00, Stephen Hemminger > wrote: >>On Fri, 20 Jul 2018 20:14:43 +0300 >>Nikolay Aleksandrov wrote: >> >>> >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. >> >>Then change store function to take a char *? > >Sure, will do. Acrually I can't change sysfs_ops store prototype. That is the reason the bond also casts the ptr.
Re: [Bridge] [PATCH net-next 1/2] net: bridge: add support for raw sysfs port options
On Fri, 20 Jul 2018 20:47:26 +0300 Nikolay Aleksandrov wrote: > On July 20, 2018 8:26:36 PM GMT+03:00, Nikolay Aleksandrov > wrote: > >On July 20, 2018 8:20:44 PM GMT+03:00, Stephen Hemminger > > wrote: > >>On Fri, 20 Jul 2018 20:14:43 +0300 > >>Nikolay Aleksandrov wrote: > >> > >>> >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. > >> > >>Then change store function to take a char *? > > > >Sure, will do. > > Acrually I can't change sysfs_ops store prototype. > That is the reason the bond also casts the ptr. > > > Ok.
Re: [Bridge] [PATCH net-next 1/2] net: bridge: add support for raw sysfs port options
From: Nikolay Aleksandrov Date: Fri, 20 Jul 2018 17:48:25 +0300 > + spin_lock_bh(&p->br->lock); > + ret = brport_attr->store_raw(p, (char *)buf); > + spin_unlock_bh(&p->br->lock); Please respect the const here. Have the methods do a kstrncup(); ... kfree(); sequence if they have to mangle the contents when there is a newline inside. I know the caller is passing in what was a non-const char pointer, I've looked at the implementation, but it might be that way forever. I looked at all other sysfs writes that need to do this \n --> \0 mangling and they either copy the string first into a static buffer or they do the kstrncup() thing. Thank you.
Re: [Bridge] [PATCH net-next 1/2] net: bridge: add support for raw sysfs port options
On 22/07/18 09:27, David Miller wrote: > From: Nikolay Aleksandrov > Date: Fri, 20 Jul 2018 17:48:25 +0300 > >> +spin_lock_bh(&p->br->lock); >> +ret = brport_attr->store_raw(p, (char *)buf); >> +spin_unlock_bh(&p->br->lock); > > Please respect the const here. > > Have the methods do a kstrncup(); ... kfree(); sequence if they have > to mangle the contents when there is a newline inside. > > I know the caller is passing in what was a non-const char pointer, > I've looked at the implementation, but it might be that way forever. > > I looked at all other sysfs writes that need to do this \n --> \0 > mangling and they either copy the string first into a static buffer > or they do the kstrncup() thing. > > Thank you. > Okay, will do. I'll also prepare a separate patch for the bonding, it does a cast like this in bonding_sysfs_store_option(). Thanks
