Linux Ethernet Bridging - Jul 2018 - [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.

On Fri, 20 Jul 2018 20:14:43 +0300
Nikolay Aleksandrov <nikolay at cumulusnetworks.com> 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 *?