Heads up --- Thinking about UDP and tunneling
Bruce Evans
brde at optusnet.com.au
Sat Dec 13 02:42:00 PST 2008
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
More information about the freebsd-net
mailing list