In approximately one week, I plan to commit the conversion of the if_name and if_unit members of struct ifnet to if_xname. Initially, this was the sum total of the commit, but code requiring knowledge of the underlying driver name and unit number necessitated the addition of two more members, if_dname and if_dunit. The current patch is available at): http://people.freebsd.org/~brooks/patches/xname.diff The diffs to sys/net/if_var.h and sys/net/if.c are included below along with an example of the driver conversion required in most cases. The working copy of the changes in in perforce at //depot/brooks/xname. I have been running this on my laptop for over a year now. It has also been tested on amd64 and sparc64 machines and compiles on alpha. However, I don't have access to anything but Ethernet and 802.11 adapters so testing on other interface types would be appreciated. Previous versions of this patch have been reviewed. I don't anticipate serious breaking on any systems since the changes are generally mechanical in nature and in the year-plus I've been hacking on them I've only created one non-bootable kernel. Some FAQs regarding this change: Q: Where was this change discussed? A: -net and -arch. re_at_ approved it in principle at BSDCon. Q: Why do this? A: Several reasons. - The primary motivation is to allow interface renaming. To do this we need to store the name in a type of storage we can modify, hence the use of an array of length IFNAMSIZ in struct ifnet. We also need to stop tracking the interface by its name. - NetBSD and OpenBSD compatibility. They switched to if_xname 5-6 years ago. - Breaking the "<drivername><unitnumber>" naming scheme will allow us more freedom to give interfaces sensible names. For instance, creating an interface named fxp0.10 count create a vlan interface on fxp0 receiving traffic tagged "10". The ef(4) device and stf(4) are both examples of devices where this naming scheme doesn't work. Q: Won't this make ipfw slow to match interfaces? A: Not for most people. When interface rules are currently matched, the unit is checked and then if it succeeds, the name is strcmp'd. Now, we just check the name. If you have many interfaces with the same name, this may create a slow down. This could be remedied with an ipfw interface change to use the if_index, but I choose not to do that for now. The other case is if the unit is a wild card. Since the goal is to break the name+unit naming scheme, I decided to use fnmatch in this case. You can now specify full shell globs for interface names in ipfw. This will be slower then a strcmp, but vastly more powerful since now you could match "fxp[124]" instead of "fxp*". -- Brooks diff -ru /usr/p4/freebsd/sys/net/if_var.h ./sys/net/if_var.h --- /usr/p4/freebsd/sys/net/if_var.h Fri Oct 17 15:15:56 2003 +++ ./sys/net/if_var.h Fri Oct 17 15:15:58 2003 _at__at_ -84,6 +84,8 _at__at_ #include <sys/mutex.h> /* XXX */ #include <sys/event.h> /* XXX */ +#define IF_DUNIT_NONE -1 + TAILQ_HEAD(ifnethead, ifnet); /* we use TAILQs so that the order of */ TAILQ_HEAD(ifaddrhead, ifaddr); /* instantiation is preserved in the list */ TAILQ_HEAD(ifprefixhead, ifprefix); _at__at_ -128,14 +130,15 _at__at_ */ struct ifnet { void *if_softc; /* pointer to driver state */ - char *if_name; /* name, e.g. ``en'' or ``lo'' */ TAILQ_ENTRY(ifnet) if_link; /* all struct ifnets are chained */ + char if_xname[IFNAMSIZ]; /* external name (name + unit) */ + const char *if_dname; /* driver name */ + int if_dunit; /* unit or IF_DUNIT_NONE */ struct ifaddrhead if_addrhead; /* linked list of addresses per if */ struct klist if_klist; /* events attached to this if */ int if_pcount; /* number of promiscuous listeners */ struct bpf_if *if_bpf; /* packet filter structure */ u_short if_index; /* numeric abbreviation for this if */ - short if_unit; /* sub-unit for lower level driver */ short if_timer; /* time 'til if_watchdog called */ u_short if_nvlans; /* number of active vlans */ int if_flags; /* up/down, broadcast, etc. */ _at__at_ -442,6 +445,7 _at__at_ int if_delmulti(struct ifnet *, struct sockaddr *); void if_detach(struct ifnet *); void if_down(struct ifnet *); +void if_initname(struct ifnet *, const char *, int); int if_printf(struct ifnet *, const char *, ...) __printflike(2, 3); void if_route(struct ifnet *, int flag, int fam); int if_setlladdr(struct ifnet *, const u_char *, int); diff -ru /usr/p4/freebsd/sys/net/if.c ./sys/net/if.c --- /usr/p4/freebsd/sys/net/if.c Thu Oct 23 10:02:58 2003 +++ ./sys/net/if.c Thu Oct 23 10:05:29 2003 _at__at_ -290,13 +290,13 _at__at_ IFNET_RLOCK(); /* could sleep on rare error; mostly okay XXX */ TAILQ_FOREACH(ifp, &ifnet, if_link) { if (ifp->if_snd.ifq_maxlen == 0) { - printf("%s%d XXX: driver didn't set ifq_maxlen\n", - ifp->if_name, ifp->if_unit); + printf("%s XXX: driver didn't set ifq_maxlen\n", + ifp->if_xname); ifp->if_snd.ifq_maxlen = ifqmaxlen; } if (!mtx_initialized(&ifp->if_snd.ifq_mtx)) { - printf("%s%d XXX: driver didn't initialize queue mtx\n", - ifp->if_name, ifp->if_unit); + printf("%s XXX: driver didn't initialize queue mtx\n", + ifp->if_xname); mtx_init(&ifp->if_snd.ifq_mtx, "unknown", MTX_NETWORK_LOCK, MTX_DEF); } _at__at_ -326,7 +326,7 _at__at_ eaddr[0] = '\0'; break; } - snprintf(devname, 32, "%s%d", ifp->if_name, ifp->if_unit); + strlcpy(devname, ifp->if_xname, sizeof(devname)); name = net_cdevsw.d_name; i = 0; while ((resource_find_dev(&i, name, &unit, NULL, NULL)) == 0) { _at__at_ -365,7 +365,6 _at__at_ { unsigned socksize, ifasize; int namelen, masklen; - char workbuf[64]; struct sockaddr_dl *sdl; struct ifaddr *ifa; _at__at_ -398,18 +397,17 _at__at_ ifnet_byindex(ifp->if_index) = ifp; ifdev_byindex(ifp->if_index) = make_dev(&net_cdevsw, ifp->if_index, - UID_ROOT, GID_WHEEL, 0600, "%s/%s%d", - net_cdevsw.d_name, ifp->if_name, ifp->if_unit); + UID_ROOT, GID_WHEEL, 0600, "%s/%s", + net_cdevsw.d_name, ifp->if_xname); make_dev_alias(ifdev_byindex(ifp->if_index), "%s%d", net_cdevsw.d_name, ifp->if_index); - mtx_init(&ifp->if_snd.ifq_mtx, ifp->if_name, "if send queue", MTX_DEF); + mtx_init(&ifp->if_snd.ifq_mtx, ifp->if_xname, "if send queue", MTX_DEF); /* * create a Link Level name for this device */ - namelen = snprintf(workbuf, sizeof(workbuf), - "%s%d", ifp->if_name, ifp->if_unit); + namelen = strlen(ifp->if_xname); #define _offsetof(t, m) ((int)((caddr_t)&((t *)0)->m)) masklen = _offsetof(struct sockaddr_dl, sdl_data[0]) + namelen; socksize = masklen + ifp->if_addrlen; _at__at_ -424,7 +422,7 _at__at_ sdl = (struct sockaddr_dl *)(ifa + 1); sdl->sdl_len = socksize; sdl->sdl_family = AF_LINK; - bcopy(workbuf, sdl->sdl_data, namelen); + bcopy(ifp->if_xname, sdl->sdl_data, namelen); sdl->sdl_nlen = namelen; sdl->sdl_index = ifp->if_index; sdl->sdl_type = ifp->if_type; _at__at_ -863,8 +861,7 _at__at_ for (ifc = LIST_FIRST(&if_cloners); ifc != NULL && count != 0; ifc = LIST_NEXT(ifc, ifc_list), count--, dst += IFNAMSIZ) { - strncpy(outbuf, ifc->ifc_name, IFNAMSIZ); - outbuf[IFNAMSIZ - 1] = '\0'; /* sanity */ + strlcpy(outbuf, ifc->ifc_name, IFNAMSIZ); error = copyout(outbuf, dst, IFNAMSIZ); if (error) break; _at__at_ -1625,8 +1622,8 _at__at_ ifr.ifr_flagshigh = ifp->if_flags >> 16; error = (*ifp->if_ioctl)(ifp, SIOCSIFFLAGS, (caddr_t)&ifr); if (error == 0) { - log(LOG_INFO, "%s%d: promiscuous mode %s\n", - ifp->if_name, ifp->if_unit, + log(LOG_INFO, "%s: promiscuous mode %s\n", + ifp->if_xname, (ifp->if_flags & IFF_PROMISC) ? "enabled" : "disabled"); rt_ifmsg(ifp); } else { _at__at_ -1655,18 +1652,14 _at__at_ ifrp = ifc->ifc_req; IFNET_RLOCK(); /* could sleep XXX */ TAILQ_FOREACH(ifp, &ifnet, if_link) { - char workbuf[64]; - int ifnlen, addrs; + int addrs; if (space < sizeof(ifr)) break; - ifnlen = snprintf(workbuf, sizeof(workbuf), - "%s%d", ifp->if_name, ifp->if_unit); - if(ifnlen + 1 > sizeof ifr.ifr_name) { + if (strlcpy(ifr.ifr_name, ifp->if_xname, sizeof(ifr.ifr_name)) + >= sizeof(ifr.ifr_name)) { error = ENAMETOOLONG; break; - } else { - strcpy(ifr.ifr_name, workbuf); } addrs = 0; _at__at_ -2001,13 +1994,30 _at__at_ return ifma; } +/* + * The name argument must be a pointer to storage which will last as + * long as the interface does. For physical devices, the result of + * device_get_name(dev) is a good choice and for pseudo-devices a + * static string works well. + */ +void +if_initname(struct ifnet *ifp, const char *name, int unit) +{ + ifp->if_dname = name; + ifp->if_dunit = unit; + if (unit != IF_DUNIT_NONE) + snprintf(ifp->if_xname, IFNAMSIZ, "%s%d", name, unit); + else + strlcpy(ifp->if_xname, name, IFNAMSIZ); +} + int if_printf(struct ifnet *ifp, const char * fmt, ...) { va_list ap; int retval; - retval = printf("%s%d: ", ifp->if_name, ifp->if_unit); + retval = printf("%s: ", ifp->if_xname); va_start(ap, fmt); retval += vprintf(fmt, ap); va_end(ap); diff -ru /usr/p4/freebsd/sys/pci/if_dc.c ./sys/pci/if_dc.c --- /usr/p4/freebsd/sys/pci/if_dc.c Thu Oct 23 10:02:59 2003 +++ ./sys/pci/if_dc.c Thu Oct 23 10:05:50 2003 _at__at_ -2228,8 +2228,7 _at__at_ ifp = &sc->arpcom.ac_if; ifp->if_softc = sc; - ifp->if_unit = unit; - ifp->if_name = "dc"; + if_initname(ifp, device_get_name(dev), device_get_unit(dev)); /* XXX: bleah, MTU gets overwritten in ether_ifattach() */ ifp->if_mtu = ETHERMTU; ifp->if_flags = IFF_BROADCAST | IFF_SIMPLEX | IFF_MULTICAST; -- Any statement of the form "X is the one, true Y" is FALSE. PGP fingerprint 655D 519C 26A7 82E7 2529 9BF0 5D8E 8BE9 F238 1AD4
This archive was generated by hypermail 2.4.0 : Wed May 19 2021 - 11:37:26 UTC