[PATCH] vlan: set sysfs device_type to 'vlan'

[PATCH] vlan: set sysfs device_type to 'vlan'

[Doug Goldstein]

Sets the sysfs device_type to 'vlan' for udev. This makes it easier for
applications that query network information via udev to identify vlans
instead of using strrchr().

Signed-off-by: Doug Goldstein <cardoe@cardoe.com>
---

Pre-patch output:
swanson ~ # udevadm info -q all -p /sys/devices/virtual/net/br0.4
P: /devices/virtual/net/br0.4
E: UDEV_LOG=3
E: DEVPATH=/devices/virtual/net/br0.4
E: INTERFACE=br0.4
E: IFINDEX=6
E: SUBSYSTEM=net

Post-patch output:
swanson ~ # udevadm info -q all -p /sys/devices/virtual/net/br0.4
P: /devices/virtual/net/br0.4
E: UDEV_LOG=3
E: DEVPATH=/devices/virtual/net/br0.4
E: DEVTYPE=vlan
E: INTERFACE=br0.4
E: IFINDEX=12
E: SUBSYSTEM=net

The intent was to match other virtual network types such as bridges.
swanson ~ # udevadm info -q all -p /sys/devices/virtual/net/br0
P: /devices/virtual/net/br0
E: UDEV_LOG=3
E: DEVPATH=/devices/virtual/net/br0
E: DEVTYPE=bridge
E: INTERFACE=br0
E: IFINDEX=5
E: SUBSYSTEM=net

---
 net/8021q/vlan_dev.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index 4024424..78a8744 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -531,6 +531,10 @@ static const struct header_ops vlan_header_ops = {
 	.parse	 = eth_header_parse,
 };
 
+static struct device_type vlan_type = {
+	.name	= "vlan",
+};
+
 static const struct net_device_ops vlan_netdev_ops;
 
 static int vlan_dev_init(struct net_device *dev)
@@ -579,6 +583,8 @@ static int vlan_dev_init(struct net_device *dev)
 
 	dev->netdev_ops = &vlan_netdev_ops;
 
+	SET_NETDEV_DEVTYPE(dev, &vlan_type);
+
 	if (is_vlan_dev(real_dev))
 		subclass = 1;
 
-- 
1.7.8.6

Re: [PATCH] vlan: set sysfs device_type to 'vlan'

[David Miller]

From: Doug Goldstein <cardoe@cardoe.com>
Date: Mon, 22 Oct 2012 00:53:57 -0500

> Sets the sysfs device_type to 'vlan' for udev. This makes it easier for
> applications that query network information via udev to identify vlans
> instead of using strrchr().
> 
> Signed-off-by: Doug Goldstein <cardoe@cardoe.com>

You're extremely misguided.  This change, in fact, makes it ten times
harder for such applications to query such devices.

Because now the application has to decide whether it wants to support
EVERY EXISTING SYSTEM OUT THERE or not.  Hundreds of millions of Linux
systems do not provide this attribute.

Applications have to handle the case of not having the 'vlan' device
type available attribute essentially forever.

So providing it in new kernels provides zero value whatsoever.

I'm not applying this patch, sorry.

Re: [PATCH] vlan: set sysfs device_type to 'vlan'

[Kay Sievers]

On Tue, Oct 23, 2012 at 8:36 AM, David Miller <davem@davemloft.net> wrote:
> From: Doug Goldstein <cardoe@cardoe.com>
> Date: Mon, 22 Oct 2012 00:53:57 -0500
>
>> Sets the sysfs device_type to 'vlan' for udev. This makes it easier for
>> applications that query network information via udev to identify vlans
>> instead of using strrchr().
>>
>> Signed-off-by: Doug Goldstein <cardoe@cardoe.com>
>
> You're extremely misguided.  This change, in fact, makes it ten times
> harder for such applications to query such devices.

That makes not much sense, really. Every new interface would fall into
that category. At least I can't see any mis-guidance here. The other
devtypes for the major netif types are not that much older.

> Because now the application has to decide whether it wants to support
> EVERY EXISTING SYSTEM OUT THERE or not.  Hundreds of millions of Linux
> systems do not provide this attribute.
>
> Applications have to handle the case of not having the 'vlan' device
> type available attribute essentially forever.

Which is an entirely separate issue, and not a technical reason not to
add new interfaces which are already in use for most other types of
netifs.

> So providing it in new kernels provides zero value whatsoever.

It sure does provide a value. The kernel can efficiently filter
uevents in the socket with this available. All other major types of
netdevs support that too, it's just a matter of completeness. For that
reason, it looks useful to me.

> I'm not applying this patch, sorry.

That's just sad. Not that I really care about that functionality, but
your reasoning is absolutely not transparent.

Kay

Re: [PATCH] vlan: set sysfs device_type to 'vlan'

[David Miller]

From: Kay Sievers <kay@vrfy.org>
Date: Tue, 23 Oct 2012 12:34:11 +0200

> On Tue, Oct 23, 2012 at 8:36 AM, David Miller <davem@davemloft.net> wrote:
>> From: Doug Goldstein <cardoe@cardoe.com>
>> Date: Mon, 22 Oct 2012 00:53:57 -0500
>>
>>> Sets the sysfs device_type to 'vlan' for udev. This makes it easier for
>>> applications that query network information via udev to identify vlans
>>> instead of using strrchr().
>>>
>>> Signed-off-by: Doug Goldstein <cardoe@cardoe.com>
>>
>> You're extremely misguided.  This change, in fact, makes it ten times
>> harder for such applications to query such devices.
> 
> That makes not much sense, really. Every new interface would fall into
> that category. At least I can't see any mis-guidance here. The other
> devtypes for the major netif types are not that much older.

Only interfaces which provide a facility available in another way
fall into this category.

Thanks for the scarecrow, but no.

Re: [PATCH] vlan: set sysfs device_type to 'vlan'

[Ben Greear]

On 10/22/2012 11:36 PM, David Miller wrote:
> From: Doug Goldstein <cardoe@cardoe.com>
> Date: Mon, 22 Oct 2012 00:53:57 -0500
>
>> Sets the sysfs device_type to 'vlan' for udev. This makes it easier for
>> applications that query network information via udev to identify vlans
>> instead of using strrchr().
>>
>> Signed-off-by: Doug Goldstein <cardoe@cardoe.com>
>
> You're extremely misguided.  This change, in fact, makes it ten times
> harder for such applications to query such devices.

If the application doesn't care, it can use the old way (which at least
for me, involves string-comparing the driver name ethtool returns, which
sucks at best).

And applications that care might suddenly have more features, or be more
efficient when running on newer kernels..

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH] vlan: set sysfs device_type to 'vlan'

[Ben Hutchings]

On Tue, 2012-10-23 at 09:14 -0700, Ben Greear wrote:
> On 10/22/2012 11:36 PM, David Miller wrote:
> > From: Doug Goldstein <cardoe@cardoe.com>
> > Date: Mon, 22 Oct 2012 00:53:57 -0500
> >
> >> Sets the sysfs device_type to 'vlan' for udev. This makes it easier for
> >> applications that query network information via udev to identify vlans
> >> instead of using strrchr().
> >>
> >> Signed-off-by: Doug Goldstein <cardoe@cardoe.com>
> >
> > You're extremely misguided.  This change, in fact, makes it ten times
> > harder for such applications to query such devices.
> 
> If the application doesn't care, it can use the old way (which at least
> for me, involves string-comparing the driver name ethtool returns, which
> sucks at best).

The 'old way' that has only worked since Linux 2.6.29 (3.5 years)?

The 'right way' seems to be to query for VLAN information through
netlink.  But that has only worked since Linux 2.6.23 (5 years ago).
The real 'old way' is to use SIOCGIFVLAN. :-/

> And applications that care might suddenly have more features, or be more
> efficient when running on newer kernels..

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH] vlan: set sysfs device_type to 'vlan'

[Ben Greear]

On 10/23/2012 03:52 PM, Ben Hutchings wrote:
> On Tue, 2012-10-23 at 09:14 -0700, Ben Greear wrote:
>> On 10/22/2012 11:36 PM, David Miller wrote:
>>> From: Doug Goldstein <cardoe@cardoe.com>
>>> Date: Mon, 22 Oct 2012 00:53:57 -0500
>>>
>>>> Sets the sysfs device_type to 'vlan' for udev. This makes it easier for
>>>> applications that query network information via udev to identify vlans
>>>> instead of using strrchr().
>>>>
>>>> Signed-off-by: Doug Goldstein <cardoe@cardoe.com>
>>>
>>> You're extremely misguided.  This change, in fact, makes it ten times
>>> harder for such applications to query such devices.
>>
>> If the application doesn't care, it can use the old way (which at least
>> for me, involves string-comparing the driver name ethtool returns, which
>> sucks at best).
>
> The 'old way' that has only worked since Linux 2.6.29 (3.5 years)?
>
> The 'right way' seems to be to query for VLAN information through
> netlink.  But that has only worked since Linux 2.6.23 (5 years ago).
> The real 'old way' is to use SIOCGIFVLAN. :-/

Oh, I have the SIOCGIFVLAN fallback in place too.

But none of this is easy from a shell script, where reading a sysfs
file is quite easy.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH] vlan: set sysfs device_type to 'vlan'

[Doug Goldstein]

On Tue, Oct 23, 2012 at 1:36 AM, David Miller <davem@davemloft.net> wrote:
> From: Doug Goldstein <cardoe@cardoe.com>
> Date: Mon, 22 Oct 2012 00:53:57 -0500
>
>> Sets the sysfs device_type to 'vlan' for udev. This makes it easier for
>> applications that query network information via udev to identify vlans
>> instead of using strrchr().
>>
>> Signed-off-by: Doug Goldstein <cardoe@cardoe.com>
>
> You're extremely misguided.  This change, in fact, makes it ten times
> harder for such applications to query such devices.
>
> Because now the application has to decide whether it wants to support
> EVERY EXISTING SYSTEM OUT THERE or not.  Hundreds of millions of Linux
> systems do not provide this attribute.
>
> Applications have to handle the case of not having the 'vlan' device
> type available attribute essentially forever.
>
> So providing it in new kernels provides zero value whatsoever.
>
> I'm not applying this patch, sorry.

Dave,

As others on this thread have discussed there are other merits to the
patch as well. Yes you are correct that netlink is the preferred
method to gather information about network devices but the point was
really to make the sysfs layout more correct a matter of completeness.
As Ben Greear pointed out this would allow shell scripts and other
scripting languages to better detect vlans. Kay pointed out that this
would allow better uevent filters in the future as well. So there are
some merits to this patch. If you'd prefer I can resubmit with a
better commit message entailing the reasons behind it.

Thanks.
-- 
Doug Goldstein

Re: [PATCH] vlan: set sysfs device_type to 'vlan'

[David Miller]

From: Doug Goldstein <cardoe@cardoe.com>
Date: Sun, 4 Nov 2012 23:45:56 -0600

> As Ben Greear pointed out this would allow shell scripts and other
> scripting languages to better detect vlans. Kay pointed out that this
> would allow better uevent filters in the future as well. So there are
> some merits to this patch. If you'd prefer I can resubmit with a
> better commit message entailing the reasons behind it.

For the thousandth time:

All of those scripts and other users of this new facility will
need to have backup code to detect vlan devices when this
sysfs thing is not present.

They are already to determine this information already, and
they alreayd have to be ugly to handle EVERY EXISTING KERNEL
ON THE PLANET.

So the effective value is zero.

Re: [PATCH] vlan: set sysfs device_type to 'vlan'

[Doug Goldstein]

On Sun, Nov 4, 2012 at 11:53 PM, David Miller <davem@davemloft.net> wrote:
> From: Doug Goldstein <cardoe@cardoe.com>
> Date: Sun, 4 Nov 2012 23:45:56 -0600
>
>> As Ben Greear pointed out this would allow shell scripts and other
>> scripting languages to better detect vlans. Kay pointed out that this
>> would allow better uevent filters in the future as well. So there are
>> some merits to this patch. If you'd prefer I can resubmit with a
>> better commit message entailing the reasons behind it.
>
> For the thousandth time:
>
> All of those scripts and other users of this new facility will
> need to have backup code to detect vlan devices when this
> sysfs thing is not present.
>
> They are already to determine this information already, and
> they alreayd have to be ugly to handle EVERY EXISTING KERNEL
> ON THE PLANET.
>
> So the effective value is zero.

By this argument we shouldn't ever improve any API or add new syscalls
since we'll have to have fallback code to handle the old interfaces
when the new ones aren't available.

Since everything already has the existing implementations to handle
every existing kernel on the planet then this patch doesn't harm
anything and should I want to write a shell script that only works on
Linux 3.8 (assuming this patch is in there) and is only forward
looking then I can use the more complete sysfs.

So the effective penalty is zero and you get a more complete sysfs,
which is where you're value is.

-- 
Doug Goldstein

Re: [PATCH] vlan: set sysfs device_type to 'vlan'

[David Miller]

From: Doug Goldstein <cardoe@cardoe.com>
Date: Mon, 5 Nov 2012 00:19:19 -0600

> By this argument we shouldn't ever improve any API or add new syscalls
> since we'll have to have fallback code to handle the old interfaces
> when the new ones aren't available.

It makes sense to add new APIs when existing mechanisms are too
difficult _AND_ code doesn't already exist that works reasonable well
with existing facilities.