Re: [RFC PATCH bridge 5/5] bridge: Add sysfs interface to display VLANS

[Vlad Yasevich <vyasevic@redhat.com>]

On 08/30/2012 10:44 AM, Michael S. Tsirkin wrote:
> On Thu, Aug 30, 2012 at 10:36:34AM -0400, Vlad Yasevich wrote:
>> On 08/30/2012 10:26 AM, Michael S. Tsirkin wrote:
>>> On Thu, Aug 30, 2012 at 10:05:16AM -0400, Vlad Yasevich wrote:
>>>> On 08/30/2012 08:27 AM, Michael S. Tsirkin wrote:
>>>>> On Thu, Aug 23, 2012 at 03:29:55PM -0400, Vlad Yasevich wrote:
>>>>>> Add a binary sysfs file that will dump out vlans currently configured on the
>>>>>> port.
>>
>> I initially though of creating a sysfs object per vlan.  That would
>> have made it easy to see which vlans are configured without any
>> tools.
>> But that could result in a lot of objects being created, so I abandoned it.
>>
>> I did think about a text interface, but due to a page of output
>> limitation, I didn't go that route.  The reason is that if someone
>> cats the file, they may not see all the vlans configured.  So I
>> decided on the binary interface, since a binary interface with a
>> tool to read it could avoid the single page limitation.
>>
>> -vlad
>
> Maybe it's not needed in sysfs then - expose it to
> brctl or whatever.
>

brctl uses sysfs for almost everything any more :)

-vlad

>>>
>>>
>>>>>
>>>>>> ---
>>>>>>   include/linux/if_bridge.h |    1 +
>>>>>>   net/bridge/br_if.c        |   34 ++++++++++++++++++++++++++++++++++
>>>>>>   net/bridge/br_private.h   |    2 ++
>>>>>>   net/bridge/br_sysfs_if.c  |   28 ++++++++++++++++++++++++++++
>>>>>>   4 files changed, 65 insertions(+), 0 deletions(-)
>>>>>>
>>>>>> diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
>>>>>> index ab750dd..d0f869b 100644
>>>>>> --- a/include/linux/if_bridge.h
>>>>>> +++ b/include/linux/if_bridge.h
>>>>>> @@ -20,6 +20,7 @@
>>>>>>   #define SYSFS_BRIDGE_PORT_SUBDIR "brif"
>>>>>>   #define SYSFS_BRIDGE_PORT_ATTR	"brport"
>>>>>>   #define SYSFS_BRIDGE_PORT_LINK	"bridge"
>>>>>> +#define SYSFS_BRIDGE_PORT_VLANS "vlans"
>>>>>>
>>>>>>   #define BRCTL_VERSION 1
>>>>>>
>>>>>> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
>>>>>> index 90c1038..3963748 100644
>>>>>> --- a/net/bridge/br_if.c
>>>>>> +++ b/net/bridge/br_if.c
>>>>>> @@ -510,6 +510,40 @@ int br_del_port_vlan(struct net_bridge_port *p, unsigned long vlan)
>>>>>>   	return 0;
>>>>>>   }
>>>>>>
>>>>>> +size_t br_port_fill_vlans(struct net_bridge_port *p, char* buf,
>>>>>> +			unsigned long max, unsigned long skip)
>>>>>> +{
>>>>>> +	unsigned long *map;
>>>>>> +	unsigned short *vid = (unsigned short *)buf;
>>>>>> +	unsigned short i;
>>>>>> +	int num = 0;
>>>>>> +
>>>>>> +	if (skip > (VLAN_N_VID+1))
>>>>>> +		return -EINVAL;
>>>>>> +
>>>>>> +	memset(buf, 0, max * sizeof(unsigned short));
>>>>>
>>>>> Isn't max is in bytes? why is this safe?
>>>>>
>>>>>> +
>>>>>> +	rcu_read_lock();
>>>>>> +	map = rcu_dereference(p->vlan_map);
>>>>>> +	if (!map)
>>>>>> +		goto out;
>>>>>> +
>>>>>> +	for (i = skip + 1; i < VLAN_N_VID + 1; i++) {
>>>>>
>>>>> Isn't skip in bytes too? Why do you compare it to i which is
>>>>> in dwords?
>>>>>
>>>>>> +		if (test_bit(i, map)) {
>>>>>> +			if (num > max)
>>>>>> +				goto out;
>>>>>> +
>>>>>> +			*vid = i-1;
>>>>>> +			vid++;
>>>>>> +			num++;
>>>>>> +		}
>>>>>> +	}
>>>>>> +out:
>>>>>> +	rcu_read_unlock();
>>>>>> +
>>>>>> +	return num*sizeof(unsigned short);
>>>>>> +}
>>>>>> +
>>>>>>   void __net_exit br_net_exit(struct net *net)
>>>>>>   {
>>>>>>   	struct net_device *dev;
>>>>>> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
>>>>>> index 5639c1c..cf95cd7 100644
>>>>>> --- a/net/bridge/br_private.h
>>>>>> +++ b/net/bridge/br_private.h
>>>>>> @@ -404,6 +404,8 @@ extern netdev_features_t br_features_recompute(struct net_bridge *br,
>>>>>>   	netdev_features_t features);
>>>>>>   extern int br_set_port_vlan(struct net_bridge_port *p, unsigned long vid);
>>>>>>   extern int br_del_port_vlan(struct net_bridge_port *p, unsigned long vid);
>>>>>> +extern size_t br_port_fill_vlans(struct net_bridge_port *p, char *buf,
>>>>>> +				unsigned long max, unsigned long skip);
>>>>>>
>>>>>>   /* br_input.c */
>>>>>>   extern int br_handle_frame_finish(struct sk_buff *skb);
>>>>>> diff --git a/net/bridge/br_sysfs_if.c b/net/bridge/br_sysfs_if.c
>>>>>> index 13b36bd..a81e2ef 100644
>>>>>> --- a/net/bridge/br_sysfs_if.c
>>>>>> +++ b/net/bridge/br_sysfs_if.c
>>>>>> @@ -234,6 +234,29 @@ const struct sysfs_ops brport_sysfs_ops = {
>>>>>>   };
>>>>>>
>>>>>>   /*
>>>>>> + * Export the vlan table for a given port as a binary file.
>>>>>> + * The records are unsgined shorts.
>>>>>> + *
>>>>>> + * Returns the number of bytes read.
>>>>>> + */
>>>>>> +static ssize_t brport_vlans_read(struct file *filp, struct kobject *kobj,
>>>>>> +				struct bin_attribute *bin_attr,
>>>>>> +				char *buf, loff_t off, size_t count)
>>>>>> +{
>>>>>> +	struct net_bridge_port *p = to_brport(kobj);
>>>>>> +
>>>>>> +	return br_port_fill_vlans(p, buf,
>>>>>> +				count/sizeof(unsigned short),
>>>>>> +				off/sizeof(unsigned short));
>>>>>> +}
>>>>>> +
>>>>>> +static struct bin_attribute port_vlans = {
>>>>>> +	.attr = { .name = SYSFS_BRIDGE_PORT_VLANS,
>>>>>> +		  .mode = S_IRUGO, },
>>>>>> +	.read = brport_vlans_read,
>>>>>> +};
>>>>>> +
>>>>>> +/*
>>>>>>    * Add sysfs entries to ethernet device added to a bridge.
>>>>>>    * Creates a brport subdirectory with bridge attributes.
>>>>>>    * Puts symlink in bridge's brif subdirectory
>>>>>> @@ -255,6 +278,11 @@ int br_sysfs_addif(struct net_bridge_port *p)
>>>>>>   			return err;
>>>>>>   	}
>>>>>>
>>>>>> +	err = sysfs_create_bin_file(&p->kobj, &port_vlans);
>>>>>> +	if (err) {
>>>>>> +		return err;
>>>>>> +	}
>>>>>> +
>>>>>>   	strlcpy(p->sysfs_name, p->dev->name, IFNAMSIZ);
>>>>>>   	return sysfs_create_link(br->ifobj, &p->kobj, p->sysfs_name);
>>>>>>   }
>>>>>> --
>>>>>> 1.7.7.6
>>>>>>
>>>>>> --
>>>>>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>>>>>> the body of a message to majordomo@vger.kernel.org
>>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html