On July 20, 2018 6:57:25 PM GMT+03:00, Stephen Hemminger <stephen@xxxxxxxxxxxxxxxxxx> wrote:
>On Fri, 20 Jul 2018 17:48:25 +0300
>Nikolay Aleksandrov <nikolay@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx>
>> ---
>> 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.
