Add firmware label support to iproute2

-----Original Message-----
From: Stephen Hemminger [mailto:[email protected]]
Sent: Thursday, August 12, 2010 11:43 PM
To: K, Narendra
Cc: [email protected]; Domsch, Matt; Rose, Charles; Hargrave, Jordan
Subject: Re: [PATCH] Add firmware label support to iproute2

ip/ipaddress.c static int ipaddr_list_or_flush(int argc, char **argv, int flush)

Quoted 4 lines · On Thu, 12 Aug 2010 12:35:37 -0500 Narendra K <[email protected]> wrote:

ip/ipaddress.c +3 -0

+#ifndef LIBNETDEVNAME_PRESENT
 			filter_dev = *argv;
+#else
+			if (netdev_alias_to_kernelname(*argv,

0)

ip/ipaddress.c static int ipaddr_list_or_flush(int argc, char **argv, int flush) · line 805

Quoted 4 lines

ip/ipaddress.c +3 -0

+				show_firmware_alias_usage(*argv);
+			filter_dev = kernel_name;
+#endif
 		}

like the kernel, I don't like ifdef cases in main code. You should put in stub inline that returns appropriate error.

What happens if alias matches existing interface name?

Quoted 8 lines

-----Original Message-----
From: Stephen Hemminger [mailto:[email protected]]
Sent: Thursday, August 12, 2010 11:43 PM
To: K, Narendra
Cc: [email protected]; Domsch, Matt; Rose, Charles; Hargrave, Jordan
Subject: Re: [PATCH] Add firmware label support to iproute2

Quoted 7 lines

ip/ipaddress.c static int ipaddr_list_or_flush(int argc, char **argv, int flush)

Quoted 4 lines · On Thu, 12 Aug 2010 12:35:37 -0500 Narendra K <[email protected]> wrote:

ip/ipaddress.c +3 -0

+#ifndef LIBNETDEVNAME_PRESENT
 			filter_dev = *argv;
+#else
+			if (netdev_alias_to_kernelname(*argv,

kernel_name) <

Quoted 9 lines

0)

ip/ipaddress.c static int ipaddr_list_or_flush(int argc, char **argv, int flush) · line 805

Quoted 4 lines

ip/ipaddress.c +3 -0

+				show_firmware_alias_usage(*argv);
+			filter_dev = kernel_name;
+#endif
 		}

like the kernel, I don't like ifdef cases in main code. You should put in stub inline that returns appropriate error.

Stephen,

Thanks for the suggestions. I will rework the patch and send a revised version soon.

Quoted 1 line

What happens if alias matches existing interface name?

If the user inputs an existing 'ethN' name, the function 'netdev_alias_to_kernelaname' will return it as it is. If the user inputs 'Embedded NIC N', then the function would do a sysfs lookup and return the corresponding 'ethN' name.

The netdev_alias_to_kernelname should only happen after normal lookup failed.

Also how does ifindex to name mapping work?

Quoted 1 line · Stephen Hemminger

The netdev_alias_to_kernelname should only happen after normal lookup failed.

Stephen, can you enlighten me as to the "right" way to do interface name lookups? While I can still find examples of parsing /proc/net/dev, or globbing /sys/class/net/*, I expect these aren't the preferred method anymore. Your own iproute2 suite uses RTM_GETLINK netlink calls, though for the seeming simple case of "give me a list of all interfaces", your path through there is far more capable (and complex) than I would hope to need.

Quoted 10 lines · On Thu, 19 Aug 2010 16:33:14 -0500 Matt Domsch <[email protected]> wrote:

Quoted 1 line · Stephen Hemminger

The netdev_alias_to_kernelname should only happen after normal lookup failed.

Stephen, can you enlighten me as to the "right" way to do interface name lookups? While I can still find examples of parsing /proc/net/dev, or globbing /sys/class/net/*, I expect these aren't the preferred method anymore. Your own iproute2 suite uses RTM_GETLINK netlink calls, though for the seeming simple case of "give me a list of all interfaces", your path through there is far more capable (and complex) than I would hope to need.

There is no magic right way. We have to support multiple interfaces. I am really concerned that all this alias stuff will turn into a disaster when there are 10,000 interfaces (Vlans). The kernel has lots of tables and hashes to handle this but if the utilities are doing a dumb scan of all names it will not work.

Also burying logic in an external library seems problematic as well. The original sysfs library was a disaster for this. I want this to work but it has to have a simple interface that is not trying to hide things.

Quoted 10 lines · On Thu, 19 Aug 2010 16:33:14 -0500 Matt Domsch <[email protected]> wrote:

Quoted 1 line · Stephen Hemminger

The netdev_alias_to_kernelname should only happen after normal lookup failed.

Stephen, can you enlighten me as to the "right" way to do interface name lookups? While I can still find examples of parsing /proc/net/dev, or globbing /sys/class/net/*, I expect these aren't the preferred method anymore. Your own iproute2 suite uses RTM_GETLINK netlink calls, though for the seeming simple case of "give me a list of all interfaces", your path through there is far more capable (and complex) than I would hope to need.

There is no magic right way. We have to support multiple interfaces. I am really concerned that all this alias stuff will turn into a disaster when there are 10,000 interfaces (Vlans). The kernel has lots of tables and hashes to handle this but if the utilities are doing a dumb scan of all names it will not work.

From 86b8ab8d24df722073d1118659c1eb269c5d9dd7 Mon Sep 17 00:00:00 2001

Quoted 14 lines · Stephen Hemminger

Quoted 10 lines · On Thu, 19 Aug 2010 16:33:14 -0500 Matt Domsch <[email protected]> wrote:

Quoted 1 line · Stephen Hemminger

The netdev_alias_to_kernelname should only happen after normal lookup failed.

Stephen, can you enlighten me as to the "right" way to do interface name lookups? While I can still find examples of parsing /proc/net/dev, or globbing /sys/class/net/*, I expect these aren't the preferred method anymore. Your own iproute2 suite uses RTM_GETLINK netlink calls, though for the seeming simple case of "give me a list of all interfaces", your path through there is far more capable (and complex) than I would hope to need.

Quoted 5 lines

There is no magic right way. We have to support multiple interfaces. I am really concerned that all this alias stuff will turn into a disaster when there are 10,000 interfaces (Vlans). The kernel has lots of tables and hashes to handle this but if the utilities are doing a dumb scan of all names it will not work.

We can remove the dumb scan of names if we expose the labels in a second way. For consideration of the approach:

Quoted 1 line

From 86b8ab8d24df722073d1118659c1eb269c5d9dd7 Mon Sep 17 00:00:00 2001

From: Matt Domsch <[email protected]>
Date: Wed, 25 Aug 2010 15:31:01 -0500
Subject: [PATCH] create /sys/firmware/pci for firmware-exposed label lookup

libnetdevname needs to be able to look up a (PCI) device by label. It currently does this by glob()ing /sys/class/net/*/device/label to find all network interfaces that have a PCI device label exposed. There can be arbitrarily many network interfaces, making this a very expensive operation.

This patch creates a new directory /sys/firmware/pci. This directory contains symlinks, named for the PCI device labels that are exposed in the 'label' file of each PCI device already. Here is an example on a Dell PowerEdge R610 server:

$ ls -l /sys/firmware/pci/
total 0
lrwxrwxrwx. 1 root root 0 2010-08-25 15:28 Embedded NIC 1 -> ../../devices/pci0000:00/0000:00:01.0/0000:01:00.0/
lrwxrwxrwx. 1 root root 0 2010-08-25 15:28 Embedded NIC 2 -> ../../devices/pci0000:00/0000:00:01.0/0000:01:00.1/
lrwxrwxrwx. 1 root root 0 2010-08-25 15:28 Embedded NIC 3 -> ../../devices/pci0000:00/0000:00:03.0/0000:02:00.0/
lrwxrwxrwx. 1 root root 0 2010-08-25 15:28 Embedded NIC 4 -> ../../devices/pci0000:00/0000:00:03.0/0000:02:00.1/
lrwxrwxrwx. 1 root root 0 2010-08-25 15:28 Embedded Video -> ../../devices/pci0000:00/0000:00:1e.0/0000:09:03.0/
lrwxrwxrwx. 1 root root 0 2010-08-25 15:28 Integrated SAS -> ../../devices/pci0000:00/0000:00:1c.0/0000:03:00.0/

$ cat /sys/devices/pci0000:00/0000:00:01.0/0000:01:00.0/label
Embedded NIC 1

Using this, libnetdevname can remove it's glob() call, and instead can look up the PCI device corresponding to a given label directly. It can then find the network interface name for that PCI device by looking in the net/ directory of that device.

This makes no attempt to deal with SMBIOS that reports the same label for two different PCI devices (if such were to exist).

Signed-off-by Matt Domsch <[email protected]>

---
 drivers/pci/pci-label.c |   66 +++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 66 insertions(+), 0 deletions(-)

drivers/pci/pci-label.c +59 -0

index 111500e..a48eedb 100644
@@ -19,6 +19,7 @@
 #include <linux/pci_ids.h>
 #include <linux/module.h>
 #include <linux/device.h>
+#include <linux/string.h>
 #include "pci.h"
 
 enum smbios_attr_enum {

@@ -131,13 +132,78 @@ pci_remove_smbiosname_file(struct pci_dev *pdev)
 	sysfs_remove_group(&pdev->dev.kobj, &smbios_attr_group);
 }
 
+static struct kobject *pci_label_kobj;
+
+static int pci_label_kobj_init(void)
+{
+	pci_label_kobj = kobject_create_and_add("pci", firmware_kobj);
+	if (pci_label_kobj == NULL)
+		return -ENOMEM;
+	return 0;
+}
+
+static int
+pci_create_smbios_label_symlink(struct pci_dev *pdev)
+{
+	char *label, *p;
+	mode_t mode;
+	int ret=-ENOMEM;
+
+	if (!pdev)
+		return -ENODEV;
+	if (pci_label_kobj == NULL)
+		if (pci_label_kobj_init())
+			return -ENOMEM;
+	label = kmalloc(PAGE_SIZE, GFP_KERNEL);
+	if (label == NULL)
+		return -ENOMEM;
+	mode = find_smbios_instance_string(pdev, label, SMBIOS_ATTR_LABEL_SHOW);
+	if (mode == 0) {
+		ret = -ENODEV;
+		goto out_free;
+	}
+	p = strim(label);
+	ret = sysfs_create_link_nowarn(pci_label_kobj, &pdev->dev.kobj, p);
+out_free:
+	kfree(label);
+	return ret;
+}
+
+static void
+pci_delete_smbios_label_symlink(struct pci_dev *pdev)
+{
+	char *label, *p;
+	mode_t mode;
+
+	if (!pdev)
+		return;
+	if (pci_label_kobj == NULL)
+		return;
+	label = kmalloc(PAGE_SIZE, GFP_KERNEL);
+	if (label == NULL)
+		return;
+	mode = find_smbios_instance_string(pdev, label, SMBIOS_ATTR_LABEL_SHOW);
+	if (mode == 0) {
+		goto out_free;
+	}
+	p = strim(label);
+	sysfs_delete_link(pci_label_kobj, &pdev->dev.kobj, p);
+
+out_free:

Quoted 5 lines · Matt Domsch

libnetdevname needs to be able to look up a (PCI) device by label. It currently does this by glob()ing /sys/class/net/*/device/label to find all network interfaces that have a PCI device label exposed. There can be arbitrarily many network interfaces, making this a very expensive operation.

So you want to add code to the kernel to make userspace have an easier 'find' path? I'm all for making stuff easier, but just how "expensive" is such an operation today?

Quoted 14 lines · Stephen Hemminger

Quoted 10 lines · On Thu, 19 Aug 2010 16:33:14 -0500 Matt Domsch <[email protected]> wrote:

Quoted 1 line · Stephen Hemminger

The netdev_alias_to_kernelname should only happen after normal lookup failed.

Stephen, can you enlighten me as to the "right" way to do interface name lookups? While I can still find examples of parsing /proc/net/dev, or globbing /sys/class/net/*, I expect these aren't the preferred method anymore. Your own iproute2 suite uses RTM_GETLINK netlink calls, though for the seeming simple case of "give me a list of all interfaces", your path through there is far more capable (and complex) than I would hope to need.

Quoted 5 lines

There is no magic right way. We have to support multiple interfaces. I am really concerned that all this alias stuff will turn into a disaster when there are 10,000 interfaces (Vlans). The kernel has lots of tables and hashes to handle this but if the utilities are doing a dumb scan of all names it will not work.

On Wed, 25 Aug 2010 17:03:23 -0500

Is it really a good idea to have to change every utility that could alter network devices? There is iproute2, iputils, tcpdump, wireshark, quagga, snmp, ... Many of the utilities come from a BSD world, and will be less likely to accept some Linux specific wart.

I have lost faith in this library wrapper support everywhere method. Let's just keep the firmware stuff in udev. If the user wants to have a policy that renames device from eth0 to "Embedded BIOS LAN1" then do it in udev. Or if you want to keep the ethX naming convention and stuff the firmware label into ifAlias or other sysfs field so it can be displayed that will be not a big issue.

-----Original Message-----
From: [email protected] [mailto:netdev- [email protected]] On Behalf Of Matt Domsch
Sent: August 26, 2010 11:01 AM
To: Stephen Hemminger
Cc: [email protected]; [email protected];

[email protected]; [email protected]; linux- [email protected]
Subject: Re: [PATCH] Add firmware label support to iproute2

Quoted 1 line · Stephen Hemminger

On Wed, 25 Aug 2010 17:03:23 -0500

Quoted 2 lines · Stephen Hemminger

Is it really a good idea to have to change every utility that could alter network devices? There is iproute2, iputils, tcpdump,

wireshark,

Quoted 2 lines

quagga, snmp, ... Many of the utilities come from a BSD world, and will be less likely to accept some Linux specific wart.

I agree, I don't want to have to change all those userspace apps either. We've started creating patches and are willing to do so if it will yield the result we want though.

Quoted 3 lines

I have lost faith in this library wrapper support everywhere method. Let's just keep the firmware stuff in udev. If the user wants to have a policy that renames device from eth0 to "Embedded BIOS LAN1"

then

Quoted 3 lines

do it in udev. Or if you want to keep the ethX naming convention and stuff the firmware label into ifAlias or other sysfs field so it can be displayed that will be not a big issue.

1) we remain constrained to IFNAMSIZ named arguments. There is no such constraint on BIOS-provided names. Dell BIOS presently uses 'Embedded NIC 1' which (fortunately) is 14 chars. We're cutting it awfully close already. I can't dictate to non-Dell BIOS vendors what to use as their strings, or how long to make them.

digression 1a) udev has a replace-spaces-with-underscores feature I used in
the biosdevname udev rules. That's a reasonable munging of the provided strings. Much more than that and I'm not sure we could consistently get it right.

2) there are apps which use a regexp for device names. We'd have to find and fix those too. Arguably we'd have to do this when we patch them to use libnetdevname. [1]

3) it continues to force a single naming convention for NICs, where for other types of devices we can have several independent naming conventions. I have end users who wish to name their interfaces by the BIOS label, others by the function (public, dmz, backup, storage, ...) that the network segment provides. While we could have different policies, each system can have only one policy at a time.

David Miller had suggested [2] that we could add a method to get alternate labels for devices by querying them. We've got something similar now by exporting the labels for devices in sysfs. Yea - progress!

But we can't _use_ those labels for more than display (meaning a human is doing the mapping in their heads), or to rename devices. We can't use the labels in scripts without doing the label-

Quoted 1 line

kernel

name lookups, and then using the kernel name.

I'm open to revisiting the "have udev rename devices", but I tried that with biosdevname 2 years ago, with little success. Adding in the hotplug dev team to the thread.

[1] http://marc.info/?l=linux-netdev&m5522163322610&w=2 (Marco d'Itri)
[2] http://marc.info/?l=linux-hotplug&m3793549323431&w=2

--
Matt Domsch
Technology Strategist
Dell | Office of the CTO
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

What if we extend 'IFNAMSIZ'(beyond 16 chars. Older apps don't need to worry because they have been working w/ 16 chars anyways) and also get ifalias to work in udev(Or is ifalias a bad idea?)?

Chetan

-----Original Message-----
From: Stephen Hemminger [mailto:[email protected]]
Sent: August 26, 2010 11:22 AM
To: Loke, Chetan
Cc: Matt Domsch; [email protected]; [email protected]; [email protected]; [email protected]; linux- [email protected]; [email protected]
Subject: Re: [PATCH] Add firmware label support to iproute2

Quoted 1 line · Chetan

What if we extend 'IFNAMSIZ'(beyond 16 chars. Older apps don't need

to

Quoted 1 line

worry because they have been working w/ 16 chars anyways) and also

get

Quoted 4 lines

ifalias to work in udev(Or is ifalias a bad idea?)?

Chetan

That is non-starter for the ioctl() style interface.

struct ifreq {

#define IFHWADDRLEN	6
	union
	{
		char	ifrn_name[IFNAMSIZ];		/* if name, e.g.

*/

	} ifr_ifrn;

	union {
		struct	sockaddr ifru_addr;
		struct	sockaddr ifru_dstaddr;
		struct	sockaddr ifru_broadaddr;
		struct	sockaddr ifru_netmask;
		struct  sockaddr ifru_hwaddr;
		short	ifru_flags;
		int	ifru_ivalue;
		int	ifru_mtu;
		struct  ifmap ifru_map;
		char	ifru_slave[IFNAMSIZ];	/* Just fits the size */
		char	ifru_newname[IFNAMSIZ];
		void __user *	ifru_data;
		struct	if_settings ifru_settings;
	} ifr_ifru;
};

Applications do:
strncpy(ifr.ifr_name, "my name", IFNAMSIZ); ioctl(fd, SIOCGHWADDR, &ifr)

Sorry, my bad.
I agree w/ your recommendation then -
1) stuff it in ifalias(or create a new alias if net_device::ifalias is used for snmp etc).

But how do we consume it?
  1.1) spit it out via sysfs? But Doc/sysfsrules.txt says "Accessing /sys/class/net/eth0/device is a bug in the application"

Quoted 1 line · Stephen Hemminger

On Wed, 25 Aug 2010 17:03:23 -0500

Quoted 4 lines · Stephen Hemminger

Is it really a good idea to have to change every utility that could alter network devices? There is iproute2, iputils, tcpdump, wireshark, quagga, snmp, ... Many of the utilities come from a BSD world, and will be less likely to accept some Linux specific wart.

I agree, I don't want to have to change all those userspace apps either. We've started creating patches and are willing to do so if it will yield the result we want though.

Quoted 6 lines

I have lost faith in this library wrapper support everywhere method. Let's just keep the firmware stuff in udev. If the user wants to have a policy that renames device from eth0 to "Embedded BIOS LAN1" then do it in udev. Or if you want to keep the ethX naming convention and stuff the firmware label into ifAlias or other sysfs field so it can be displayed that will be not a big issue.

1) we remain constrained to IFNAMSIZ named arguments. There is no such constraint on BIOS-provided names. Dell BIOS presently uses 'Embedded NIC 1' which (fortunately) is 14 chars. We're cutting it awfully close already. I can't dictate to non-Dell BIOS vendors what to use as their strings, or how long to make them.