| Message ID | 20100812173537.GA29784@auslistsprd01.us.dell.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | stephen hemminger |
| Headers | show |
On Thu, 12 Aug 2010 12:35:37 -0500 Narendra K <Narendra_K@dell.com> wrote: > +ifeq ($(shell test -L /usr/local/lib/libnetdevname.so; echo $$?),0) > +LDLIBS +=-lnetdevname > +DEFINES += -DLIBNETDEVNAME_PRESENT > +endif I assume any user or distro using libnetdevname will put it in the standard path not /usr/lib, don't hard code the path here. why not build a sample program like the ATM checks? -- 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 Thu, 12 Aug 2010 12:35:37 -0500 Narendra K <Narendra_K@dell.com> wrote: > +#ifndef LIBNETDEVNAME_PRESENT > filter_dev = *argv; > +#else > + if (netdev_alias_to_kernelname(*argv, kernel_name) < 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? -- 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
> -----Original Message----- > From: Stephen Hemminger [mailto:shemminger@vyatta.com] > Sent: Thursday, August 12, 2010 11:43 PM > To: K, Narendra > Cc: netdev@vger.kernel.org; Domsch, Matt; Rose, Charles; Hargrave, > Jordan > Subject: Re: [PATCH] Add firmware label support to iproute2 > > On Thu, 12 Aug 2010 12:35:37 -0500 > Narendra K <Narendra_K@dell.com> wrote: > > > +#ifndef LIBNETDEVNAME_PRESENT > > filter_dev = *argv; > > +#else > > + if (netdev_alias_to_kernelname(*argv, kernel_name) < > 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. > 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. With regards, Narendra K -- 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 Fri, 13 Aug 2010 18:06:26 +0530 <Narendra_K@Dell.com> wrote: > > -----Original Message----- > > From: Stephen Hemminger [mailto:shemminger@vyatta.com] > > Sent: Thursday, August 12, 2010 11:43 PM > > To: K, Narendra > > Cc: netdev@vger.kernel.org; Domsch, Matt; Rose, Charles; Hargrave, > > Jordan > > Subject: Re: [PATCH] Add firmware label support to iproute2 > > > > On Thu, 12 Aug 2010 12:35:37 -0500 > > Narendra K <Narendra_K@dell.com> wrote: > > > > > +#ifndef LIBNETDEVNAME_PRESENT > > > filter_dev = *argv; > > > +#else > > > + if (netdev_alias_to_kernelname(*argv, > kernel_name) < > > 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. > > > 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?
On Wed, Aug 18, 2010 at 02:41:24PM -0700, Stephen Hemminger wrote: > 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. Please advise. > Also how does ifindex to name mapping work? libnetdevname does not use ifindex at all at present. Thanks, Matt
On Thu, 19 Aug 2010 16:33:14 -0500 Matt Domsch <Matt_Domsch@Dell.com> wrote: > On Wed, Aug 18, 2010 at 02:41:24PM -0700, Stephen Hemminger wrote: > > 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. -- 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 Thu, Aug 19, 2010 at 02:53:08PM -0700, Stephen Hemminger wrote: > On Thu, 19 Aug 2010 16:33:14 -0500 > Matt Domsch <Matt_Domsch@Dell.com> wrote: > > > On Wed, Aug 18, 2010 at 02:41:24PM -0700, Stephen Hemminger wrote: > > > 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. I believe you proposed a lookup algorithm in each app, something like: if exists(passed device name) use it else if (kernelname=netdev_alias_to_kernelname(passed device name)) use kernelname so I'm looking for the "right" way to do these existance tests. I agree iterating through all the interfaces should be avoided. > 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. the library right now is all of: 422 netdevname.c 38 netdevname.h with another 100 line app to print the mappings in both directions. The interface is pretty simple too: extern int netdev_alias_to_kernelname (const char *alias, char *kernelname); struct netdev_alias { char *name; const char *namespace_name; int is_descriptive; /* this may disappear yet */ struct netdev_alias *next; }; extern int netdev_kernelname_to_aliases (const char *, struct netdev_alias **); extern void free_netdev_aliases (struct netdev_alias *); The common app usage will be to call netdev_alias_to_kernelname(). Only those apps that want to display the alternate names would use the struct or other 2 functions. Even though it's a small library, I do think it makes sense to make it a library. I expect to add additional "namespaces" (e.g. "fw:Embedded NIC 1" is the name of a device provided by the firmware. As I mentioned in my talk at LinuxCon last week (slides at http://domsch.com/linux/linuxcon10/) there could be other valid name providers, and we'd like them to be usable without re-patching all the applications again. As always, I'm open to ideas. Thanks, Matt
diff --git a/Makefile b/Makefile index 77a85c6..ee82640 100644 --- a/Makefile +++ b/Makefile @@ -35,6 +35,11 @@ YACCFLAGS = -d -t -v LDLIBS += -L../lib -lnetlink -lutil +ifeq ($(shell test -L /usr/local/lib/libnetdevname.so; echo $$?),0) +LDLIBS +=-lnetdevname +DEFINES += -DLIBNETDEVNAME_PRESENT +endif + SUBDIRS=lib ip tc misc netem genl LIBNETLINK=../lib/libnetlink.a ../lib/libutil.a diff --git a/ip/ipaddress.c b/ip/ipaddress.c index 3a411b1..6fafa2e 100644 --- a/ip/ipaddress.c +++ b/ip/ipaddress.c @@ -28,6 +28,10 @@ #include <linux/if_arp.h> #include <linux/sockios.h> +#ifdef LIBNETDEVNAME_PRESENT +#include <netdevname.h> +#endif + #include "rt_names.h" #include "utils.h" #include "ll_map.h" @@ -712,6 +716,10 @@ static int ipaddr_list_or_flush(int argc, char **argv, int flush) char *filter_dev = NULL; int no_link = 0; +#ifdef LIBNETDEVNAME_PRESENT + char kernel_name[IFNAMSIZ]; +#endif + ipaddr_reset_filter(oneline); filter.showqueue = 1; @@ -787,7 +795,13 @@ static int ipaddr_list_or_flush(int argc, char **argv, int flush) usage(); if (filter_dev) duparg2("dev", *argv); +#ifndef LIBNETDEVNAME_PRESENT filter_dev = *argv; +#else + if (netdev_alias_to_kernelname(*argv, kernel_name) < 0) + show_firmware_alias_usage(*argv); + filter_dev = kernel_name; +#endif } argv++; argc--; } diff --git a/ip/iplink.c b/ip/iplink.c index cb2c4f5..e9f70e2 100644 --- a/ip/iplink.c +++ b/ip/iplink.c @@ -28,6 +28,10 @@ #include <sys/ioctl.h> #include <linux/sockios.h> +#ifdef LIBNETDEVNAME_PRESENT +#include <netdevname.h> +#endif + #include "rt_names.h" #include "utils.h" #include "ip_common.h" @@ -703,6 +707,10 @@ static int do_set(int argc, char **argv) char *newname = NULL; int htype, halen; +#ifdef LIBNETDEVNAME_PRESENT + char kernel_name[IFNAMSIZ]; +#endif + while (argc > 0) { if (strcmp(*argv, "up") == 0) { mask |= IFF_UP; @@ -798,7 +806,13 @@ static int do_set(int argc, char **argv) usage(); if (dev) duparg2("dev", *argv); +#ifndef LIBNETDEVNAME_PRESENT dev = *argv; +#else + if (netdev_alias_to_kernelname(*argv, kernel_name) < 0) + show_firmware_alias_usage(*argv); + dev = kernel_name; +#endif } argc--; argv++; } diff --git a/ip/ipmaddr.c b/ip/ipmaddr.c index 44ffdfc..b5c8380 100644 --- a/ip/ipmaddr.c +++ b/ip/ipmaddr.c @@ -26,6 +26,10 @@ #include <linux/if_arp.h> #include <linux/sockios.h> +#ifdef LIBNETDEVNAME_PRESENT +#include <netdevname.h> +#endif + #include "rt_names.h" #include "utils.h" @@ -245,6 +249,9 @@ static int multiaddr_list(int argc, char **argv) { struct ma_info *list = NULL; +#ifdef LIBNETDEVNAME_PRESENT + char kernel_name[IFNAMSIZ]; +#endif if (!filter.family) filter.family = preferred_family; @@ -257,7 +264,13 @@ static int multiaddr_list(int argc, char **argv) usage(); if (filter.dev) duparg2("dev", *argv); +#ifndef LIBNETDEVNAME_PRESENT filter.dev = *argv; +#else + if (netdev_alias_to_kernelname(*argv, kernel_name) < 0) + show_firmware_alias_usage(*argv); + filter.dev = kernel_name; +#endif } argv++; argc--; } @@ -289,7 +302,12 @@ int multiaddr_modify(int cmd, int argc, char **argv) NEXT_ARG(); if (ifr.ifr_name[0]) duparg("dev", *argv); +#ifndef LIBNETDEVNAME_PRESENT strncpy(ifr.ifr_name, *argv, IFNAMSIZ); +#else + if (netdev_alias_to_kernelname(*argv, ifr.ifr_name) < 0) + show_firmware_alias_usage(*argv); +#endif } else { if (matches(*argv, "address") == 0) { NEXT_ARG(); diff --git a/lib/ll_map.c b/lib/ll_map.c index b8b49aa..1476255 100644 --- a/lib/ll_map.c +++ b/lib/ll_map.c @@ -19,9 +19,15 @@ #include <netinet/in.h> #include <string.h> +#ifdef LIBNETDEVNAME_PRESENT +#include <netdevname.h> +#include <linux/if.h> +#endif + #include "libnetlink.h" #include "ll_map.h" + extern unsigned int if_nametoindex (const char *); struct idxmap @@ -163,8 +169,18 @@ unsigned ll_name_to_index(const char *name) int i; unsigned idx; +#ifdef LIBNETDEVNAME_PRESENT + char kernel_name[IFNAMSIZ]; +#endif if (name == NULL) return 0; + +#ifdef LIBNETDEVNAME_PRESENT + if (netdev_alias_to_kernelname(name, kernel_name) < 0) + show_firmware_alias_usage(name); + strncpy(name, kernel_name, IFNAMSIZ); +#endif + if (icache && strcmp(name, ncache) == 0) return icache; for (i=0; i<16; i++) { diff --git a/tc/f_fw.c b/tc/f_fw.c index 219b404..244bc8a 100644 --- a/tc/f_fw.c +++ b/tc/f_fw.c @@ -20,6 +20,11 @@ #include <arpa/inet.h> #include <string.h> #include <linux/if.h> /* IFNAMSIZ */ + +#ifdef LIBNETDEVNAME_PRESENT +#include <netdevname.h> +#endif + #include "utils.h" #include "tc_util.h" @@ -39,6 +44,10 @@ static int fw_parse_opt(struct filter_util *qu, char *handle, int argc, char **a __u32 mask = 0; int mask_set = 0; +#ifdef LIBNETDEVNAME_PRESENT + char kernel_name[IFNAMSIZ]; +#endif + memset(&tp, 0, sizeof(tp)); if (handle) { @@ -100,7 +109,13 @@ static int fw_parse_opt(struct filter_util *qu, char *handle, int argc, char **a fprintf(stderr, "Illegal indev\n"); return -1; } +#ifndef LIBNETDEVNAME_PRESENT strncpy(d, *argv, sizeof (d) - 1); +#else + if (netdev_alias_to_kernelname(*argv, kernel_name) < 0) + show_firmware_alias_usage(*argv); + strncpy(d, kernel_name, sizeof (d) - 1); +#endif addattr_l(n, MAX_MSG, TCA_FW_INDEV, d, strlen(d) + 1); } else if (strcmp(*argv, "help") == 0) { explain(); diff --git a/tc/tc_class.c b/tc/tc_class.c index 9d4eea5..bff90f8 100644 --- a/tc/tc_class.c +++ b/tc/tc_class.c @@ -21,6 +21,11 @@ #include <string.h> #include <math.h> +#ifdef LIBNETDEVNAME_PRESENT +#include <netdevname.h> +#include <linux/if.h> +#endif + #include "utils.h" #include "tc_util.h" #include "tc_common.h" @@ -52,6 +57,10 @@ int tc_class_modify(int cmd, unsigned flags, int argc, char **argv) char d[16]; char k[16]; +#ifdef LIBNETDEVNAME_PRESENT + char kernel_name[IFNAMSIZ]; +#endif + memset(&req, 0, sizeof(req)); memset(&est, 0, sizeof(est)); memset(d, 0, sizeof(d)); @@ -67,7 +76,13 @@ int tc_class_modify(int cmd, unsigned flags, int argc, char **argv) NEXT_ARG(); if (d[0]) duparg("dev", *argv); +#ifndef LIBNETDEVNAME_PRESENT strncpy(d, *argv, sizeof(d)-1); +#else + if (netdev_alias_to_kernelname(*argv, kernel_name) < 0) + show_firmware_alias_usage(*argv); + strncpy(d, kernel_name, sizeof (d) - 1); +#endif } else if (strcmp(*argv, "classid") == 0) { __u32 handle; NEXT_ARG(); @@ -237,6 +252,10 @@ int tc_class_list(int argc, char **argv) struct tcmsg t; char d[16]; +#ifdef LIBNETDEVNAME_PRESENT + char kernel_name[IFNAMSIZ]; +#endif + memset(&t, 0, sizeof(t)); t.tcm_family = AF_UNSPEC; memset(d, 0, sizeof(d)); @@ -246,7 +265,13 @@ int tc_class_list(int argc, char **argv) NEXT_ARG(); if (d[0]) duparg("dev", *argv); +#ifndef LIBNETDEVNAME_PRESENT strncpy(d, *argv, sizeof(d)-1); +#else + if (netdev_alias_to_kernelname(*argv, kernel_name) < 0) + show_firmware_alias_usage(*argv); + strncpy(d, kernel_name, sizeof (d) - 1); +#endif } else if (strcmp(*argv, "qdisc") == 0) { NEXT_ARG(); if (filter_qdisc) diff --git a/tc/tc_filter.c b/tc/tc_filter.c index 919c57c..e144f9d 100644 --- a/tc/tc_filter.c +++ b/tc/tc_filter.c @@ -21,6 +21,11 @@ #include <string.h> #include <linux/if_ether.h> +#ifdef LIBNETDEVNAME_PRESENT +#include <netdevname.h> +#include <linux/if.h> +#endif + #include "rt_names.h" #include "utils.h" #include "tc_util.h" @@ -61,6 +66,10 @@ int tc_filter_modify(int cmd, unsigned flags, int argc, char **argv) char k[16]; struct tc_estimator est; +#ifdef LIBNETDEVNAME_PRESENT + char kernel_name[IFNAMSIZ]; +#endif + memset(&req, 0, sizeof(req)); memset(&est, 0, sizeof(est)); memset(d, 0, sizeof(d)); @@ -80,7 +89,13 @@ int tc_filter_modify(int cmd, unsigned flags, int argc, char **argv) NEXT_ARG(); if (d[0]) duparg("dev", *argv); +#ifndef LIBNETDEVNAME_PRESENT strncpy(d, *argv, sizeof(d)-1); +#else + if (netdev_alias_to_kernelname(*argv, kernel_name) < 0) + show_firmware_alias_usage(*argv); + strncpy(d, kernel_name, sizeof (d) - 1); +#endif } else if (strcmp(*argv, "root") == 0) { if (req.t.tcm_parent) { fprintf(stderr, "Error: \"root\" is duplicate parent ID\n"); @@ -268,6 +283,10 @@ int tc_filter_list(int argc, char **argv) __u32 protocol = 0; char *fhandle = NULL; +#ifdef LIBNETDEVNAME_PRESENT + char kernel_name[IFNAMSIZ]; +#endif + memset(&t, 0, sizeof(t)); t.tcm_family = AF_UNSPEC; memset(d, 0, sizeof(d)); @@ -277,7 +296,13 @@ int tc_filter_list(int argc, char **argv) NEXT_ARG(); if (d[0]) duparg("dev", *argv); +#ifndef LIBNETDEVNAME_PRESENT strncpy(d, *argv, sizeof(d)-1); +#else + if (netdev_alias_to_kernelname(*argv, kernel_name) < 0) + show_firmware_alias_usage(*argv); + strncpy(d, kernel_name, sizeof (d) - 1); +#endif } else if (strcmp(*argv, "root") == 0) { if (t.tcm_parent) { fprintf(stderr, "Error: \"root\" is duplicate parent ID\n"); diff --git a/tc/tc_qdisc.c b/tc/tc_qdisc.c index c7f2988..3a9b05c 100644 --- a/tc/tc_qdisc.c +++ b/tc/tc_qdisc.c @@ -21,6 +21,10 @@ #include <string.h> #include <math.h> #include <malloc.h> +#ifdef LIBNETDEVNAME_PRESENT +#include <netdevname.h> +#include <linux/if.h> +#endif #include "utils.h" #include "tc_util.h" @@ -60,6 +64,10 @@ int tc_qdisc_modify(int cmd, unsigned flags, int argc, char **argv) char buf[TCA_BUF_MAX]; } req; +#ifdef LIBNETDEVNAME_PRESENT + char kernel_name[IFNAMSIZ]; +#endif + memset(&req, 0, sizeof(req)); memset(&stab, 0, sizeof(stab)); memset(&est, 0, sizeof(est)); @@ -76,7 +84,13 @@ int tc_qdisc_modify(int cmd, unsigned flags, int argc, char **argv) NEXT_ARG(); if (d[0]) duparg("dev", *argv); +#ifndef LIBNETDEVNAME_PRESENT strncpy(d, *argv, sizeof(d)-1); +#else + if (netdev_alias_to_kernelname(*argv, kernel_name) < 0) + show_firmware_alias_usage(*argv); + strncpy(d, kernel_name, sizeof (d) - 1); +#endif } else if (strcmp(*argv, "handle") == 0) { __u32 handle; if (req.t.tcm_handle) @@ -282,6 +296,10 @@ int tc_qdisc_list(int argc, char **argv) struct tcmsg t; char d[16]; +#ifdef LIBNETDEVNAME_PRESENT + char kernel_name[IFNAMSIZ]; +#endif + memset(&t, 0, sizeof(t)); t.tcm_family = AF_UNSPEC; memset(&d, 0, sizeof(d)); @@ -289,7 +307,13 @@ int tc_qdisc_list(int argc, char **argv) while (argc > 0) { if (strcmp(*argv, "dev") == 0) { NEXT_ARG(); +#ifndef LIBNETDEVNAME_PRESENT strncpy(d, *argv, sizeof(d)-1); +#else + if (netdev_alias_to_kernelname(*argv, kernel_name) < 0) + show_firmware_alias_usage(*argv); + strncpy(d, kernel_name, sizeof (d) - 1); +#endif #ifdef TC_H_INGRESS } else if (strcmp(*argv, "ingress") == 0) { if (t.tcm_parent) {