Date: Sat, 14 Feb 2009 18:59:57 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Luigi Rizzo <luigi@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r188578 - head/sys/netinet Message-ID: <20090214183758.I847@besplex.bde.org> In-Reply-To: <200902131514.n1DFEhft091837@svn.freebsd.org> References: <200902131514.n1DFEhft091837@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, 13 Feb 2009, Luigi Rizzo wrote:
> Log:
> Use uint32_t instead of n_long and n_time, and uint16_t instead of n_short.
> Add a note next to fields in network format.
>
> The n_* types are not enough for compiler checks on endianness, and their
> use often requires an otherwise unnecessary #include <netinet/in_systm.h>
This is too much like globally substituting uid_t with uint16_t. At
least the n_time typedef provides some correct opaqueness (while n_long
is just bogus since the network size is 4 octets and not 1 long on
64-bit machines).
> Modified: head/sys/netinet/ip.h
> ==============================================================================
> --- head/sys/netinet/ip.h Fri Feb 13 14:43:46 2009 (r188577)
> +++ head/sys/netinet/ip.h Fri Feb 13 15:14:43 2009 (r188578)
> @@ -150,10 +150,10 @@ struct ip_timestamp {
> ipt_flg:4; /* flags, see below */
> #endif
> union ipt_timestamp {
> - n_long ipt_time[1];
> + uint32_t ipt_time[1]; /* network format */
The old typedef-names also allow better formatting (since they are shorter
than 8 characters). This declaration is now indented an extra 8 characters...
> struct ipt_ta {
> struct in_addr ipt_addr;
> - n_long ipt_time;
> + uint32_t ipt_time; /* network format */
... while this one is indented inconsistently by 1 space before and after.
Similarly elsewhere.
The new comments are bogus. Surely everything here is in network format,
not just the now-annotated fields. Similarly elsewhere.
> Modified: head/sys/netinet/ip_icmp.h
> ==============================================================================
> --- head/sys/netinet/ip_icmp.h Fri Feb 13 14:43:46 2009 (r188577)
> +++ head/sys/netinet/ip_icmp.h Fri Feb 13 15:14:43 2009 (r188578)
> @@ -127,7 +131,7 @@ struct icmp {
> * ip header length.
> */
> #define ICMP_MINLEN 8 /* abs minimum */
> -#define ICMP_TSLEN (8 + 3 * sizeof (n_time)) /* timestamp */
> +#define ICMP_TSLEN (8 + 3 * sizeof (uint32_t)) /* timestamp */
The changes lose the most where they involve sizeof's. Now it is
unclear that the sizeof is of 1 timestamp. sizeof(uint32_t) is an
obfuscated spelling of 4.
This change preserves the style bug. sizeof's are not followed by a space.
> Modified: head/sys/netinet/ip_options.c
> ==============================================================================
> --- head/sys/netinet/ip_options.c Fri Feb 13 14:43:46 2009 (r188577)
> +++ head/sys/netinet/ip_options.c Fri Feb 13 15:14:43 2009 (r188578)
> @@ -105,7 +105,7 @@ ip_dooptions(struct mbuf *m, int pass)
> struct in_ifaddr *ia;
> int opt, optlen, cnt, off, code, type = ICMP_PARAMPROB, forward = 0;
> struct in_addr *sin, dst;
> - n_time ntime;
> + uint32_t ntime;
> struct sockaddr_in ipaddr = { sizeof(ipaddr), AF_INET };
>
> /* Ignore or reject packets with IP options. */
> @@ -320,7 +320,7 @@ dropit:
> break;
>
> case IPOPT_TS_TSANDADDR:
> - if (off + sizeof(n_time) +
> + if (off + sizeof(uint32_t) +
> sizeof(struct in_addr) > optlen) {
> code = &cp[IPOPT_OFFSET] - (u_char *)ip;
> goto bad;
> @@ -337,7 +337,7 @@ dropit:
> break;
>
> case IPOPT_TS_PRESPEC:
> - if (off + sizeof(n_time) +
> + if (off + sizeof(uint32_t) +
> sizeof(struct in_addr) > optlen) {
> code = &cp[IPOPT_OFFSET] - (u_char *)ip;
> goto bad;
sizeof(type) is probably the best spelling of the magic number in the above...
> @@ -355,8 +355,8 @@ dropit:
> goto bad;
> }
> ntime = iptime();
> - (void)memcpy(cp + off, &ntime, sizeof(n_time));
> - cp[IPOPT_OFFSET] += sizeof(n_time);
> + (void)memcpy(cp + off, &ntime, sizeof(uint32_t));
> + cp[IPOPT_OFFSET] += sizeof(uint32_t);
... but here we are copying the object `ntime'; sizeof(n_time) was an
obfuscated spelling of the size of that object, and sizeof(uint32_t) is
an even more obfuscated spelling. Similarly elsewhere.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20090214183758.I847>