HEADSUP if_xname incoming in one week

From: Brooks Davis <brooks_at_one-eyed-alien.net>
Date: Thu, 23 Oct 2003 19:49:14 -0700
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

Received on Thu Oct 23 2003 - 17:49:18 UTC

This archive was generated by hypermail 2.4.0 : Wed May 19 2021 - 11:37:26 UTC