| Message ID | 1350885237-12998-1-git-send-email-cardoe@cardoe.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
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. -- 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
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 -- 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
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
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. -- 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
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.
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
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.
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. -- 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
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.
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. -- 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
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;
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(-)