From owner-freebsd-net@FreeBSD.ORG Sat Dec 13 10:42:00 2008 Return-Path: Delivered-To: freebsd-net@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 6883B106564A; Sat, 13 Dec 2008 10:42:00 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail06.syd.optusnet.com.au (mail06.syd.optusnet.com.au [211.29.132.187]) by mx1.freebsd.org (Postfix) with ESMTP id E1F798FC17; Sat, 13 Dec 2008 10:41:59 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from besplex.bde.org (c122-107-112-209.carlnfd1.nsw.optusnet.com.au [122.107.112.209]) by mail06.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id mBDAftnF029828 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sat, 13 Dec 2008 21:41:56 +1100 Date: Sat, 13 Dec 2008 21:41:55 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Randall Stewart In-Reply-To: <8835B402-CD0D-4246-ABD3-F2B1BB230820@lakerest.net> Message-ID: <20081213203723.A977@besplex.bde.org> References: <49249443.8050707@elischer.org> <76CF7D15-251F-4E43-86BE-AD96F48AF123@lakerest.net> <200811201450.30016.max@love2party.net> <24BD4A21-E10D-4E09-8C33-3FCF930A0495@lakerest.net> <494157DF.6030802@FreeBSD.org> <13C9478E-CBF6-4EDA-8E78-AD76549EB844@lakerest.net> <20081213030449.F2405@delplex.bde.org> <8835B402-CD0D-4246-ABD3-F2B1BB230820@lakerest.net> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: freebsd-net , "Bruce M. Simpson" Subject: Re: Heads up --- Thinking about UDP and tunneling X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 13 Dec 2008 10:42:00 -0000 On Fri, 12 Dec 2008, Randall Stewart wrote: > Well tell you what Bruce: > > How about if I just run the WHOLE file through s9indent... > > This will fix ALL my problems.. and of course "fix" the > rest of the file too.. Any automated conversion utility is very likely to introduce more bugs than it fixes. I haven't seen any that even try to work right. You would have to inspect every change and throw out most. With a working-right utility, you wouldn't have to do this since you would direct it to make only changes that you really want, including somehow restricting the changes to your new code. > Unless you think its already in conformance (bet you > its not) :-) According to my (buggy) knfometer utilty (just indent(1) with certain options followed by comparision of the size of the diff with the size of the original file): 94.325% udp_usrreq.c 80.452% udp_var.h 93.528% udp6_usrreq.c Anything above 90% is good and indicates that most of the changes are due to bugs in the conversion utility. In fact, indent seems to be making much the same wrong changes as your s9indent (I see the patch from that in a later reply -- I might respond to that too). Here is what indent does with udp_usrreq.c (the diff is larger than 5.675% of the file since it is -u2 while 5.675% is for a plain diff): % --- udp_usrreq.c.BAK 2008-12-13 09:49:18.398640000 +0000 % +++ udp_usrreq.c 2008-12-13 09:49:18.403640000 +0000 % @@ -99,4 +99,5 @@ % #ifdef VIMAGE_GLOBALS % int udp_blackhole; % + % #endif % Bug in indent. indent knows that it doesn't understand cpp directives so it tries not to touch them, but it still botches #endif by adding a blank line (and by misformatting any comment on #endif). % @@ -107,9 +108,11 @@ % * cause problems (especially for NFS data blocks). % */ % -static int udp_cksum = 1; % +static int udp_cksum = 1; Bug in indent, same as in s9indent. indent doesn't understand the arcane rule followed by the original code here, though I have directed it to indent by 1 tab after declaration-specificers. The problem is that indent knows that there are problems doing this for declaration-specifers longer than 7 characters (since the tab couldn't give the normal indent of 8 characters then), while the above code is happy to indent all the variable names to 16 characters. % + Bug in indent. Not shared by s9indent. The problem is that indent doesn't understand the ugly SYSCTL_INT() in the next statement. It thinks that SYSCTL_INT() is a function definition and has been directed to put a blank line before function definitions. % SYSCTL_INT(_net_inet_udp, UDPCTL_CHECKSUM, checksum, CTLFLAG_RW, &udp_cksum, % 0, "compute udp checksum"); % % int udp_log_in_vain = 0; % + Same bug as above. Now the indentation is unchanged though it is inconsistent with previous indentation, since the declaration-specifier is short. s9indent breaks the indentation here, since it doesn't understand or hasn't been directed that variable names should be indented by a tab (except in auto declarations). % SYSCTL_INT(_net_inet_udp, OID_AUTO, log_in_vain, CTLFLAG_RW, % &udp_log_in_vain, 0, "Log all incoming UDP packets"); % @@ -120,5 +123,6 @@ % % u_long udp_sendspace = 9216; /* really max datagram size */ % - /* 40 1K datagrams */ % + % + /* 40 1K datagrams */ Bug in indent, same as in s9indent. indent doesn't understand that the second comment is part of the first (which is a hard thing to understand), so it makes a mess. % SYSCTL_ULONG(_net_inet_udp, UDPCTL_MAXDGRAM, maxdgram, CTLFLAG_RW, % &udp_sendspace, 0, "Maximum outgoing UDP datagram size"); % @@ -126,9 +130,9 @@ % u_long udp_recvspace = 40 * (1024 + % #ifdef INET6 % - sizeof(struct sockaddr_in6) % + sizeof(struct sockaddr_in6) % #else % - sizeof(struct sockaddr_in) % + sizeof(struct sockaddr_in) % #endif % - ); % +); indent mangles the special formatting here, the same as s9indent. It is following the strict rules here (except the final ");" should be indented by 4 characters too), but the original code has fancy formatting that intentionally breaks the rules. The strict rules are also wrong. indent doesn't really understand initializers in declarations. For function calls, it would start the 4-character continuation indent at the start of the function name, but it doesn't have a corresponding rule for initalizers and indents relative to column 0. % % SYSCTL_ULONG(_net_inet_udp, UDPCTL_RECVSPACE, recvspace, CTLFLAG_RW, % @@ -136,7 +140,8 @@ % % #ifdef VIMAGE_GLOBALS % -struct inpcbhead udb; /* from udp_var.h */ % -struct inpcbinfo udbinfo; % -struct udpstat udpstat; /* from udp_var.h */ % +struct inpcbhead udb; /* from udp_var.h */ % +struct inpcbinfo udbinfo; % +struct udpstat udpstat; /* from udp_var.h */ indent only messes up the fancy indentation of the variable names here. s9indent does this too, and also messes up the indentation of the comments. indent preserves the 40-character indentation of the comments though they might appear to be changed due to mail and markup not aligning the start of the line to a tab position. s9indent changes the comment indentation to 32 characters. I direct indent to use 40 characters (-c41 -cd41) because checking lots of KNF code (all of kern and vi and some others) indicates that 40 characters is slightly more common than 32. udp_usrreq.c apparently uses 40 so knfometer likes it. % ... % @@ -149,7 +154,8 @@ % "UDP statistics (struct udpstat, netinet/udp_var.h)"); % % -static void udp_detach(struct socket *so); % -static int udp_output(struct inpcb *, struct mbuf *, struct sockaddr *, % - struct mbuf *, struct thread *); % +static void udp_detach(struct socket *so); % +static int % +udp_output(struct inpcb *, struct mbuf *, struct sockaddr *, % + struct mbuf *, struct thread *); indent doesn't understand protoytypes and mangles their original perfect KNF formatting unless they are short. It also doesn't understand line splitting, so it doesn't change the splitting point for udp_output() in the above ("struct mbuf *" now fits on the previous line). s9indent has the same bugs. % @@ -219,5 +227,5 @@ % return; % } % -#endif /* IPSEC */ % +#endif /* IPSEC */ Bug documented above. % #ifdef MAC % if (mac_inpcb_check_deliver(inp, n) != 0) { % @@ -272,6 +280,8 @@ % struct ip save_ip; % struct sockaddr_in udp_in; % + % #ifdef IPFIREWALL_FORWARD % struct m_tag *fwd_tag; % + % #endif indent also puts a bogus blank line before ifdef. % % @@ -284,9 +294,8 @@ % * check the checksum with options still present. % */ % - if (iphlen > sizeof (struct ip)) { % + if (iphlen > sizeof(struct ip)) { Here is the first change that is actually correct. % ip_stripoptions(m, (struct mbuf *)0); % iphlen = sizeof(struct ip); % } % - % /* % * Get IP and UDP header together in first mbuf. Always omitting optional blank lines is KNF, but I don't like it before block comments. % ... % @@ -361,5 +369,5 @@ % bzero(((struct ipovly *)ip)->ih_x1, 9); % ((struct ipovly *)ip)->ih_len = uh->uh_ulen; % - uh_sum = in_cksum(m, len + sizeof (struct ip)); % + uh_sum = in_cksum(m, len + sizeof(struct ip)); % bcopy(b, ((struct ipovly *)ip)->ih_x1, 9); % } Actually correct. % @@ -433,8 +441,8 @@ % if (IN_MULTICAST(ntohl(ip->ip_dst.s_addr)) && % imo != NULL) { % - struct sockaddr_in sin; % - struct in_msource *ims; % - int blocked, mode; % - size_t idx; % + struct sockaddr_in sin; % + struct in_msource *ims; % + int blocked, mode; % + size_t idx; % % bzero(&sin, sizeof(struct sockaddr_in)); Correct again. Maybe only FreeBSD indent supports different indentation rules for local declarations. % @@ -466,7 +474,7 @@ % mode = imo->imo_mfilters[idx].imf_fmode; % if ((ims != NULL && % - mode == MCAST_EXCLUDE) || % + mode == MCAST_EXCLUDE) || % (ims == NULL && % - mode == MCAST_INCLUDE)) { % + mode == MCAST_INCLUDE)) { % #ifdef DIAGNOSTIC % if (bootverbose) { % @@ -504,5 +512,5 @@ % */ % if ((last->inp_socket->so_options & % - (SO_REUSEPORT|SO_REUSEADDR)) == 0) % + (SO_REUSEPORT | SO_REUSEADDR)) == 0) % break; % } All correct. % ... % @@ -531,5 +538,5 @@ % if (inp == NULL) { % if (udp_log_in_vain) { % - char buf[4*sizeof "123"]; % + char buf[4 * sizeof "123"]; Correct, but indent doesn't support adding the required parentheses here. % @@ -661,8 +667,7 @@ % n = V_udbinfo.ipi_count; % req->oldidx = 2 * (sizeof xig) % - + (n + n/8) * sizeof(struct xinpcb); % + + (n + n / 8) * sizeof(struct xinpcb); Correct but incomplete. indent doesn't support the style rule that requires the line split to occur after the operation instead of before. % return (0); % } % - % if (req->newptr != 0) % return (EPERM); Example of an optional blank line that should be removed. % [... lots more, mostly correct] % @@ -1278,18 +1276,18 @@ % % struct pr_usrreqs udp_usrreqs = { % - .pru_abort = udp_abort, % - .pru_attach = udp_attach, % - .pru_bind = udp_bind, % - .pru_connect = udp_connect, % - .pru_control = in_control, % - .pru_detach = udp_detach, % - .pru_disconnect = udp_disconnect, % - .pru_peeraddr = in_getpeeraddr, % - .pru_send = udp_send, % - .pru_soreceive = soreceive_dgram, % - .pru_sosend = sosend_dgram, % - .pru_shutdown = udp_shutdown, % - .pru_sockaddr = in_getsockaddr, % - .pru_sosetlabel = in_pcbsosetlabel, % - .pru_close = udp_close, % + .pru_abort = udp_abort, % + .pru_attach = udp_attach, % + .pru_bind = udp_bind, % + .pru_connect = udp_connect, % + .pru_control = in_control, % + .pru_detach = udp_detach, % + .pru_disconnect = udp_disconnect, % + .pru_peeraddr = in_getpeeraddr, % + .pru_send = udp_send, % + .pru_soreceive = soreceive_dgram, % + .pru_sosend = sosend_dgram, % + .pru_shutdown = udp_shutdown, % + .pru_sockaddr = in_getsockaddr, % + .pru_sosetlabel = in_pcbsosetlabel, % + .pru_close = udp_close, % }; indent doesn't understand the fancy formatting in the original code and makes a mess. s9indent has the same bug. To understand this, indent would probably need to understand C99 struct initializers, but indent doesn't even understand C90 prototypes yet. Once indent understands this, it could support a flag for fancy formatting of struct initializers independently of its formatting of other initializers and assignments. Bruce