Skip site navigation (1)Skip section navigation (2)
Date:      Wed, 16 Oct 2002 00:17:13 +0200
From:      Poul-Henning Kamp <phk@freebsd.org>
To:        net@freebsd.org, arch@freebsd.org
Subject:   RFC: eliminating the _IP_VHL hack.
Message-ID:  <60637.1034720233@critter.freebsd.dk>

next in thread | raw e-mail | index | archive | help

almost 7 years ago, this commit introduced the _IP_VHL hack in our
IP-stack:

] revision 1.7
] date: 1995/12/21 21:20:27;  author: wollman;  state: Exp;  lines: +5 -1
] If _IP_VHL is defined, declare a single ip_vhl member in struct ip rather
] than separate ip_v and ip_hl members.  Should have no effect on current code,
] but I'd eventually like to get rid of those obnoxious bitfields completely.

We can argue a lot about how long time we should wait for "eventually",
but I would say that 7 years is far too long, considering the status:

In the meantime absolutely no code has picked up on this idea, and source
files using the _IP_VHL hack in in the minority in our tree.

The side effect of having some source-files using the _IP_VHL hack and
some not is that sizeof(struct ip) varies from file to file, which at
best is confusing an at worst the source of some really evil bugs.

I would therefore propose to eliminate the _IP_VHL hack from the kernel
to end this state of (potential) confusion, and invite comments to the
following patch:

Index: ip.h
===================================================================
RCS file: /home/ncvs/src/sys/netinet/ip.h,v
retrieving revision 1.19
diff -u -r1.19 ip.h
--- ip.h	14 Dec 2001 19:37:32 -0000	1.19
+++ ip.h	15 Oct 2002 22:02:26 -0000
@@ -47,9 +47,6 @@
  * Structure of an internet header, naked of options.
  */
 struct ip {
-#ifdef _IP_VHL
-	u_char	ip_vhl;			/* version << 4 | header length >> 2 */
-#else
 #if BYTE_ORDER == LITTLE_ENDIAN
 	u_int	ip_hl:4,		/* header length */
 		ip_v:4;			/* version */
@@ -58,7 +55,6 @@
 	u_int	ip_v:4,			/* version */
 		ip_hl:4;		/* header length */
 #endif
-#endif /* not _IP_VHL */
 	u_char	ip_tos;			/* type of service */
 	u_short	ip_len;			/* total length */
 	u_short	ip_id;			/* identification */
@@ -72,13 +68,6 @@
 	u_short	ip_sum;			/* checksum */
 	struct	in_addr ip_src,ip_dst;	/* source and dest address */
 };
-
-#ifdef _IP_VHL
-#define	IP_MAKE_VHL(v, hl)	((v) << 4 | (hl))
-#define	IP_VHL_HL(vhl)		((vhl) & 0x0f)
-#define	IP_VHL_V(vhl)		((vhl) >> 4)
-#define	IP_VHL_BORING		0x45
-#endif
 
 #define	IP_MAXPACKET	65535		/* maximum packet size */
 
Index: ip_icmp.c
===================================================================
RCS file: /home/ncvs/src/sys/netinet/ip_icmp.c,v
retrieving revision 1.70
diff -u -r1.70 ip_icmp.c
--- ip_icmp.c	1 Aug 2002 03:53:04 -0000	1.70
+++ ip_icmp.c	15 Oct 2002 22:05:23 -0000
@@ -51,7 +51,6 @@
 #include <net/if_types.h>
 #include <net/route.h>
 
-#define _IP_VHL
 #include <netinet/in.h>
 #include <netinet/in_systm.h>
 #include <netinet/in_var.h>
@@ -128,7 +127,7 @@
 	struct ifnet *destifp;
 {
 	register struct ip *oip = mtod(n, struct ip *), *nip;
-	register unsigned oiplen = IP_VHL_HL(oip->ip_vhl) << 2;
+	register unsigned oiplen = oip->ip_hl << 2;
 	register struct icmp *icp;
 	register struct mbuf *m;
 	unsigned icmplen;
@@ -214,7 +213,8 @@
 	nip = mtod(m, struct ip *);
 	bcopy((caddr_t)oip, (caddr_t)nip, sizeof(struct ip));
 	nip->ip_len = m->m_len;
-	nip->ip_vhl = IP_VHL_BORING;
+	nip->ip_v = IPVERSION;
+	nip->ip_hl = 5;
 	nip->ip_p = IPPROTO_ICMP;
 	nip->ip_tos = 0;
 	icmp_reflect(m);
@@ -364,7 +364,7 @@
 		 * Problem with datagram; advise higher level routines.
 		 */
 		if (icmplen < ICMP_ADVLENMIN || icmplen < ICMP_ADVLEN(icp) ||
-		    IP_VHL_HL(icp->icmp_ip.ip_vhl) < (sizeof(struct ip) >> 2)) {
+		    icp->icmp_ip.ip_hl < (sizeof(struct ip) >> 2)) {
 			icmpstat.icps_badlen++;
 			goto freeit;
 		}
@@ -526,7 +526,7 @@
 		if (code > 3)
 			goto badcode;
 		if (icmplen < ICMP_ADVLENMIN || icmplen < ICMP_ADVLEN(icp) ||
-		    IP_VHL_HL(icp->icmp_ip.ip_vhl) < (sizeof(struct ip) >> 2)) {
+		    icp->icmp_ip.ip_hl < (sizeof(struct ip) >> 2)) {
 			icmpstat.icps_badlen++;
 			break;
 		}
@@ -593,7 +593,7 @@
 	struct in_ifaddr *ia;
 	struct in_addr t;
 	struct mbuf *opts = 0;
-	int optlen = (IP_VHL_HL(ip->ip_vhl) << 2) - sizeof(struct ip);
+	int optlen = (ip->ip_hl << 2) - sizeof(struct ip);
 	struct route *ro = NULL, rt;
 
 	if (!in_canforward(ip->ip_src) &&
@@ -703,7 +703,8 @@
 		 * mbuf's data back, and adjust the IP length.
 		 */
 		ip->ip_len -= optlen;
-		ip->ip_vhl = IP_VHL_BORING;
+		ip->ip_v = IPVERSION;
+		ip->ip_hl = 5;
 		m->m_len -= optlen;
 		if (m->m_flags & M_PKTHDR)
 			m->m_pkthdr.len -= optlen;
@@ -734,7 +735,7 @@
 	register int hlen;
 	register struct icmp *icp;
 
-	hlen = IP_VHL_HL(ip->ip_vhl) << 2;
+	hlen = ip->ip_hl << 2;
 	m->m_data += hlen;
 	m->m_len -= hlen;
 	icp = mtod(m, struct icmp *);
Index: ip_icmp.h
===================================================================
RCS file: /home/ncvs/src/sys/netinet/ip_icmp.h,v
retrieving revision 1.18
diff -u -r1.18 ip_icmp.h
--- ip_icmp.h	19 Mar 2002 21:25:46 -0000	1.18
+++ ip_icmp.h	15 Oct 2002 22:02:52 -0000
@@ -123,13 +123,8 @@
 #define	ICMP_TSLEN	(8 + 3 * sizeof (n_time))	/* timestamp */
 #define	ICMP_MASKLEN	12				/* address mask */
 #define	ICMP_ADVLENMIN	(8 + sizeof (struct ip) + 8)	/* min */
-#ifndef _IP_VHL
 #define	ICMP_ADVLEN(p)	(8 + ((p)->icmp_ip.ip_hl << 2) + 8)
 	/* N.B.: must separately check that ip_hl >= 5 */
-#else
-#define	ICMP_ADVLEN(p)	(8 + (IP_VHL_HL((p)->icmp_ip.ip_vhl) << 2) + 8)
-	/* N.B.: must separately check that header length >= 5 */
-#endif
 
 /*
  * Definition of type and code field values.
Index: ip_input.c
===================================================================
RCS file: /home/ncvs/src/sys/netinet/ip_input.c,v
retrieving revision 1.211
diff -u -r1.211 ip_input.c
--- ip_input.c	10 Oct 2002 12:03:36 -0000	1.211
+++ ip_input.c	15 Oct 2002 22:02:57 -0000
@@ -34,8 +34,6 @@
  * $FreeBSD: src/sys/netinet/ip_input.c,v 1.211 2002/10/10 12:03:36 maxim Exp $
  */
 
-#define	_IP_VHL
-
 #include "opt_bootp.h"
 #include "opt_ipfw.h"
 #include "opt_ipdn.h"
@@ -324,7 +322,7 @@
 
 	if (args.rule) {	/* dummynet already filtered us */
 		ip = mtod(m, struct ip *);
-		hlen = IP_VHL_HL(ip->ip_vhl) << 2;
+		hlen = ip->ip_hl << 2;
 		goto iphack ;
 	}
 
@@ -340,12 +338,12 @@
 	}
 	ip = mtod(m, struct ip *);
 
-	if (IP_VHL_V(ip->ip_vhl) != IPVERSION) {
+	if (ip->ip_v != IPVERSION) {
 		ipstat.ips_badvers++;
 		goto bad;
 	}
 
-	hlen = IP_VHL_HL(ip->ip_vhl) << 2;
+	hlen = ip->ip_hl << 2;
 	if (hlen < sizeof(struct ip)) {	/* minimum header length */
 		ipstat.ips_badhlen++;
 		goto bad;
@@ -755,7 +753,7 @@
 		ipstat.ips_reassembled++;
 		ip = mtod(m, struct ip *);
 		/* Get the header length of the reassembled packet */
-		hlen = IP_VHL_HL(ip->ip_vhl) << 2;
+		hlen = ip->ip_hl << 2;
 #ifdef IPDIVERT
 		/* Restore original checksum before diverting packet */
 		if (divert_info != 0) {
@@ -882,7 +880,7 @@
 	struct ip *ip = mtod(m, struct ip *);
 	register struct mbuf *p, *q, *nq;
 	struct mbuf *t;
-	int hlen = IP_VHL_HL(ip->ip_vhl) << 2;
+	int hlen = ip->ip_hl << 2;
 	int i, next;
 
 	/*
@@ -1020,7 +1018,7 @@
 	 */
 	q = fp->ipq_frags;
 	ip = GETIP(q);
-	if (next + (IP_VHL_HL(ip->ip_vhl) << 2) > IP_MAXPACKET) {
+	if (next + (ip->ip_hl << 2) > IP_MAXPACKET) {
 		ipstat.ips_toolong++;
 		ip_freef(head, fp);
 		return (0);
@@ -1068,8 +1066,8 @@
 	nipq--;
 	(void) m_free(dtom(fp));
 	ip_nfragpackets--;
-	m->m_len += (IP_VHL_HL(ip->ip_vhl) << 2);
-	m->m_data -= (IP_VHL_HL(ip->ip_vhl) << 2);
+	m->m_len += (ip->ip_hl << 2);
+	m->m_data -= (ip->ip_hl << 2);
 	/* some debugging cruft by sklower, below, will go away soon */
 	if (m->m_flags & M_PKTHDR)	/* XXX this should be done elsewhere */
 		m_fixhdr(m);
@@ -1193,7 +1191,7 @@
 
 	dst = ip->ip_dst;
 	cp = (u_char *)(ip + 1);
-	cnt = (IP_VHL_HL(ip->ip_vhl) << 2) - sizeof (struct ip);
+	cnt = (ip->ip_hl << 2) - sizeof (struct ip);
 	for (; cnt > 0; cnt -= optlen, cp += optlen) {
 		opt = cp[IPOPT_OPTVAL];
 		if (opt == IPOPT_EOL)
@@ -1582,14 +1580,15 @@
 	register caddr_t opts;
 	int olen;
 
-	olen = (IP_VHL_HL(ip->ip_vhl) << 2) - sizeof (struct ip);
+	olen = (ip->ip_hl << 2) - sizeof (struct ip);
 	opts = (caddr_t)(ip + 1);
 	i = m->m_len - (sizeof (struct ip) + olen);
 	bcopy(opts + olen, opts, (unsigned)i);
 	m->m_len -= olen;
 	if (m->m_flags & M_PKTHDR)
 		m->m_pkthdr.len -= olen;
-	ip->ip_vhl = IP_MAKE_VHL(IPVERSION, sizeof(struct ip) >> 2);
+	ip->ip_v = IPVERSION;
+	ip->ip_hl = sizeof(struct ip) >> 2;
 }
 
 u_char inetctlerrmap[PRC_NCMDS] = {
@@ -1686,7 +1685,7 @@
 	MGET(mcopy, M_DONTWAIT, m->m_type);
 	if (mcopy != NULL) {
 		M_COPY_PKTHDR(mcopy, m);
-		mcopy->m_len = imin((IP_VHL_HL(ip->ip_vhl) << 2) + 8,
+		mcopy->m_len = imin((ip->ip_hl << 2) + 8,
 		    (int)ip->ip_len);
 		m_copydata(m, 0, mcopy->m_len, mtod(mcopy, caddr_t));
 #ifdef MAC
Index: ip_output.c
===================================================================
RCS file: /home/ncvs/src/sys/netinet/ip_output.c,v
retrieving revision 1.165
diff -u -r1.165 ip_output.c
--- ip_output.c	23 Sep 2002 08:56:24 -0000	1.165
+++ ip_output.c	15 Oct 2002 22:03:41 -0000
@@ -34,8 +34,6 @@
  * $FreeBSD: src/sys/netinet/ip_output.c,v 1.165 2002/09/23 08:56:24 maxim Exp $
  */
 
-#define _IP_VHL
-
 #include "opt_ipfw.h"
 #include "opt_ipdn.h"
 #include "opt_ipdivert.h"
@@ -191,7 +189,7 @@
 #endif
 	if (args.rule != NULL) {	/* dummynet already saw us */
 		ip = mtod(m, struct ip *);
-		hlen = IP_VHL_HL(ip->ip_vhl) << 2 ;
+		hlen = ip->ip_hl << 2 ;
 		if (ro->ro_rt)
 			ia = ifatoia(ro->ro_rt->rt_ifa);
 		goto sendit;
@@ -210,7 +208,8 @@
 	 * Fill in IP header.
 	 */
 	if ((flags & (IP_FORWARDING|IP_RAWOUTPUT)) == 0) {
-		ip->ip_vhl = IP_MAKE_VHL(IPVERSION, hlen >> 2);
+		ip->ip_v = IPVERSION;
+		ip->ip_hl = hlen >> 2;
 		ip->ip_off &= IP_DF;
 #ifdef RANDOM_IP_ID
 		ip->ip_id = ip_randomid();
@@ -219,7 +218,7 @@
 #endif
 		ipstat.ips_localout++;
 	} else {
-		hlen = IP_VHL_HL(ip->ip_vhl) << 2;
+		hlen = ip->ip_hl << 2;
 	}
 
 	dst = (struct sockaddr_in *)&ro->ro_dst;
@@ -553,11 +552,7 @@
 
 	/* be sure to update variables that are affected by ipsec4_output() */
 	ip = mtod(m, struct ip *);
-#ifdef _IP_VHL
-	hlen = IP_VHL_HL(ip->ip_vhl) << 2;
-#else
 	hlen = ip->ip_hl << 2;
-#endif
 	if (ro->ro_rt == NULL) {
 		if ((flags & IP_ROUTETOIF) == 0) {
 			printf("ip_output: "
@@ -862,13 +857,8 @@
 		ip->ip_len = htons(ip->ip_len);
 		ip->ip_off = htons(ip->ip_off);
 		ip->ip_sum = 0;
-		if (sw_csum & CSUM_DELAY_IP) {
-			if (ip->ip_vhl == IP_VHL_BORING) {
-				ip->ip_sum = in_cksum_hdr(ip);
-			} else {
-				ip->ip_sum = in_cksum(m, hlen);
-			}
-		}
+		if (sw_csum & CSUM_DELAY_IP)
+			ip->ip_sum = in_cksum(m, hlen);
 
 		/* Record statistics for this interface address. */
 		if (!(flags & IP_FORWARDING) && ia) {
@@ -988,7 +978,8 @@
 		*mhip = *ip;
 		if (hlen > sizeof (struct ip)) {
 			mhlen = ip_optcopy(ip, mhip) + sizeof (struct ip);
-			mhip->ip_vhl = IP_MAKE_VHL(IPVERSION, mhlen >> 2);
+			mhip->ip_v = IPVERSION;
+			mhip->ip_hl = mhlen >> 2;
 		}
 		m->m_len = mhlen;
 		mhip->ip_off = ((off - hlen) >> 3) + ip->ip_off;
@@ -1012,13 +1003,8 @@
 		m->m_pkthdr.csum_flags = m0->m_pkthdr.csum_flags;
 		mhip->ip_off = htons(mhip->ip_off);
 		mhip->ip_sum = 0;
-		if (sw_csum & CSUM_DELAY_IP) {
-			if (mhip->ip_vhl == IP_VHL_BORING) {
-				mhip->ip_sum = in_cksum_hdr(mhip);
-			} else {
-				mhip->ip_sum = in_cksum(m, mhlen);
-			}
-		}
+		if (sw_csum & CSUM_DELAY_IP)
+			mhip->ip_sum = in_cksum(m, mhlen);
 		*mnext = m;
 		mnext = &m->m_nextpkt;
 		nfrags++;
@@ -1041,13 +1027,8 @@
 	ip->ip_off |= IP_MF;
 	ip->ip_off = htons(ip->ip_off);
 	ip->ip_sum = 0;
-	if (sw_csum & CSUM_DELAY_IP) {
-		if (ip->ip_vhl == IP_VHL_BORING) {
-			ip->ip_sum = in_cksum_hdr(ip);
-		} else {
-			ip->ip_sum = in_cksum(m, hlen);
-		}
-	}
+	if (sw_csum & CSUM_DELAY_IP)
+		ip->ip_sum = in_cksum(m, hlen);
 sendorfree:
 	for (m = m0; m; m = m0) {
 		m0 = m->m_nextpkt;
@@ -1097,7 +1078,7 @@
 	u_short csum, offset;
 
 	ip = mtod(m, struct ip *);
-	offset = IP_VHL_HL(ip->ip_vhl) << 2 ;
+	offset = ip->ip_hl << 2 ;
 	csum = in_cksum_skip(m, ip->ip_len, offset);
 	if (m->m_pkthdr.csum_flags & CSUM_UDP && csum == 0)
 		csum = 0xffff;
@@ -1169,7 +1150,8 @@
 	ip = mtod(m, struct ip *);
 	bcopy(p->ipopt_list, ip + 1, optlen);
 	*phlen = sizeof(struct ip) + optlen;
-	ip->ip_vhl = IP_MAKE_VHL(IPVERSION, *phlen >> 2);
+	ip->ip_v = IPVERSION;
+	ip->ip_hl = *phlen >> 2;
 	ip->ip_len += optlen;
 	return (m);
 }
@@ -1187,7 +1169,7 @@
 
 	cp = (u_char *)(ip + 1);
 	dp = (u_char *)(jp + 1);
-	cnt = (IP_VHL_HL(ip->ip_vhl) << 2) - sizeof (struct ip);
+	cnt = (ip->ip_hl << 2) - sizeof (struct ip);
 	for (; cnt > 0; cnt -= optlen, cp += optlen) {
 		opt = cp[0];
 		if (opt == IPOPT_EOL)
@@ -2025,11 +2007,7 @@
 		ip->ip_len = htons(ip->ip_len);
 		ip->ip_off = htons(ip->ip_off);
 		ip->ip_sum = 0;
-		if (ip->ip_vhl == IP_VHL_BORING) {
-			ip->ip_sum = in_cksum_hdr(ip);
-		} else {
-			ip->ip_sum = in_cksum(copym, hlen);
-		}
+		ip->ip_sum = in_cksum(copym, hlen);
 		/*
 		 * NB:
 		 * It's not clear whether there are any lingering
Index: raw_ip.c
===================================================================
RCS file: /home/ncvs/src/sys/netinet/raw_ip.c,v
retrieving revision 1.100
diff -u -r1.100 raw_ip.c
--- raw_ip.c	15 Aug 2002 18:51:26 -0000	1.100
+++ raw_ip.c	15 Oct 2002 22:06:09 -0000
@@ -59,7 +59,6 @@
 #include <net/if.h>
 #include <net/route.h>
 
-#define _IP_VHL
 #include <netinet/in.h>
 #include <netinet/in_systm.h>
 #include <netinet/in_pcb.h>
@@ -263,10 +262,10 @@
 		ip = mtod(m, struct ip *);
 		/* don't allow both user specified and setsockopt options,
 		   and don't allow packet length sizes that will crash */
-		if (((IP_VHL_HL(ip->ip_vhl) != (sizeof (*ip) >> 2))
+		if (((ip->ip_hl != (sizeof (*ip) >> 2))
 		     && inp->inp_options)
 		    || (ip->ip_len > m->m_pkthdr.len)
-		    || (ip->ip_len < (IP_VHL_HL(ip->ip_vhl) << 2))) {
+		    || (ip->ip_len < (ip->ip_hl << 2))) {
 			m_freem(m);
 			return EINVAL;
 		}
Index: tcp_subr.c
===================================================================
RCS file: /home/ncvs/src/sys/netinet/tcp_subr.c,v
retrieving revision 1.143
diff -u -r1.143 tcp_subr.c
--- tcp_subr.c	10 Oct 2002 21:41:30 -0000	1.143
+++ tcp_subr.c	15 Oct 2002 22:06:27 -0000
@@ -62,7 +62,6 @@
 #include <net/route.h>
 #include <net/if.h>
 
-#define _IP_VHL
 #include <netinet/in.h>
 #include <netinet/in_systm.h>
 #include <netinet/ip.h>
@@ -284,7 +283,8 @@
 	{
 	struct ip *ip = (struct ip *) ip_ptr;
 
-	ip->ip_vhl = IP_VHL_BORING;
+	ip->ip_v = IPVERSION;
+	ip->ip_hl = 5;
 	ip->ip_tos = 0;
 	ip->ip_len = 0;
 	ip->ip_id = 0;
@@ -371,7 +371,7 @@
 	KASSERT(tp != NULL || m != NULL, ("tcp_respond: tp and m both NULL"));
 
 #ifdef INET6
-	isipv6 = IP_VHL_V(((struct ip *)ipgen)->ip_vhl) == 6;
+	isipv6 = ((struct ip *)ipgen)->ip_v == 6;
 	ip6 = ipgen;
 #endif /* INET6 */
 	ip = ipgen;
@@ -1102,7 +1102,7 @@
 	if (ip) {
 		s = splnet();
 		th = (struct tcphdr *)((caddr_t)ip 
-				       + (IP_VHL_HL(ip->ip_vhl) << 2));
+				       + (ip->ip_hl << 2));
 		INP_INFO_WLOCK(&tcbinfo);
 		inp = in_pcblookup_hash(&tcbinfo, faddr, th->th_dport,
 		    ip->ip_src, th->th_sport, 0, NULL);

-- 
Poul-Henning Kamp       | UNIX since Zilog Zeus 3.20
phk@FreeBSD.ORG         | TCP/IP since RFC 956
FreeBSD committer       | BSD since 4.3-tahoe
Never attribute to malice what can adequately be explained by incompetence.

To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-arch" in the body of the message




Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?60637.1034720233>