Date: Sat, 13 Dec 2008 22:28:43 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Randall Stewart <rrs@lakerest.net> Cc: freebsd-net <freebsd-net@freebsd.org>, "Bruce M. Simpson" <bms@freebsd.org> Subject: Re: Heads up --- Thinking about UDP and tunneling Message-ID: <20081213214209.L977@besplex.bde.org> In-Reply-To: <E5493B85-6578-49C5-9BE8-41157FB2E26D@lakerest.net> References: <D72E9703-C8E7-4A21-A71E-A4B4C2D7E8F4@lakerest.net> <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> <E5493B85-6578-49C5-9BE8-41157FB2E26D@lakerest.net>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, 12 Dec 2008, Randall Stewart wrote:
> 1) I went ahead and fixed the comments.. even added a ! instead of :-(
>
> 2) No problem using func_t.. changed to that.. seems nicer :-D
>
> 3) Removed an extra cr or two you pointed out.. hopefully got them all.
OK.
> 4) I disagree with you on the cast... its not ugly.. it prevents us
> from having to have a per_pcb structure for UDP when all we need
> is one pointer. As I said in my first post.. it seemed like to much
> overhead, creating a zone for a single pointer... I am not adverse to
> casts .. but of course I guess I am just an old fart who has been around
> to many years writing code :-)
This is actually more broken than I first thought. inp_ppcb is "void *"
but is abused to hold a function pointer. This gives undefined behaviour
with or without the cast. The actual behaviour is apparently to work
on all supported machines, but to suppress printing of a diagnostic with
the cast. On ia64, function pointers are very different from "void *",
but they can still be represented as "void *" after a conversion. I think
ia64 function pointers are implemented as something like an integer index
into a table of the actual pointers. Since 2^32 function pointers should
be enough for anyone, 64-bit "void *" is more than large enough to represent
them.
If inp_ppcb were a generic function pointer, then a conversion would still
be needed, but it doesn't need to be a cast. Simple assignment without
the cast must work. However, I think we have warning flags set to such
a high level that there would be warnings for the type change done by
assignment, and the cast would be needed anyway to suppress the warning.
I don't like this. Casts normally do suppress warnings and this hides
errors too.
Of course, saving space is a valid reason for this hack.
>
> 5) I ran s9indent.. and of course it found a lot of other things you missed..
> but that
> is the way of s9indent I have found :-D
Not surprising that it has bugs :-).
Apart from the types of errors mentioned in my previous mail, it does the
following unformatting:
[hard carriage returns removed from diff]
% Index: netinet/udp_usrreq.c
% ===================================================================
% --- netinet/udp_usrreq.c (revision 185928)
% +++ netinet/udp_usrreq.c (working copy)
% @@ -97,7 +97,8 @@
% */
%
% #ifdef VIMAGE_GLOBALS
% -int udp_blackhole;
% +int udp_blackhole;
% +
% #endif
Hmm, it mangles #endif the same as indent(1), so it seems to be just a
wrapper for indent(1), just with slightly worse directives than my
knfometer.
[% ...]
I don't notice many more problems than mentioned in my previous mail.
The slightly worse directives are -c33 instead of -c41 and whatever
directive it is that breaks the tab indentation for udp_blackhole
above. BTW, I used to use that directive in knfometer too. It was
needed because most declarations are local, but old FreeBSD indent
and gnu indent don't support different formatting for local declarations,
so the directive that gave the least mangling was the one that preserved
the formatting of local declarations. I eventually modified FreeBSD
indent to support the different formatting. Not having this is one of
the main problems that makes gnu indent not directly usable for formatting
to KNF (gnu indent is generally better but has fewer features -- surprising
for a gnu utility).
% @@ -387,7 +395,8 @@
% uh->uh_dport = ntohs(next_hop->sin_port);
%
% /*
% - * Remove the tag from the packet. We don't need it anymore.
% + * Remove the tag from the packet. We don't need it
% + * anymore.
% */
% m_tag_delete(m, fwd_tag);
% }
No need to change this. Another bug in indent is that it has no flexibility
for preserving formatting that is good enough. Normally you don't want
large block comments reformatted. The line length parameter only works
for reformatting comments, but minor changes in it normally lead to complete
reformatting of block comments. knfometer uses directives to suppress
reformatting of all block comments although this misses some necessary
reformatting.
% @@ -414,10 +423,11 @@
% inp->inp_faddr.s_addr != ip->ip_src.s_addr)
% continue;
% /*
% - * XXX: Do not check source port of incoming datagram
% - * unless inp_connect() has been called to bind the
% - * fport part of the 4-tuple; the source could be
% - * trying to talk to us with an ephemeral port.
% + * XXX: Do not check source port of incoming
% + * datagram unless inp_connect() has been called to
% + * bind the fport part of the 4-tuple; the source
% + * could be trying to talk to us with an ephemeral
% + * port.
% */
Again, the original formatting was good enough. The programmer may have
done right near-justification manually or using a better utility than
indent.
[... most changes continue to be for correctly formatted block comments]
% @@ -488,41 +500,72 @@
% struct mbuf *n;
%
% n = m_copy(m, 0, M_COPYALL);
% - if (n != NULL)
% - udp_append(last, ip, n, iphlen +
% - sizeof(struct udphdr), &udp_in);
% - INP_RUNLOCK(last);
% + if (last->inp_ppcb == NULL) {
% + if (n != NULL)
% + udp_append(last, ip, n, iphlen +
% + sizeof(struct udphdr), &udp_in);
Still have several too-line lines.
indent doesn't support reformatting long lines except in comments.
% Index: netinet/udp_var.h
% ===================================================================
% --- netinet/udp_var.h (revision 185928)
% +++ netinet/udp_var.h (working copy)
% @@ -38,9 +38,10 @@
% * UDP kernel structures and variables.
% */
% struct udpiphdr {
% - struct ipovly ui_i; /* overlaid ip structure */
% - struct udphdr ui_u; /* udp header */
% + struct ipovly ui_i; /* overlaid ip structure */
% + struct udphdr ui_u; /* udp header */
% };
indent with suitable directives can avoid this mangling, but neither it nor
style(9) know the core rule that the indentation may be excessive (to
make struct member names line up despite verbose declaration-specifiers)
provided at least 10% of the declaration-specifiers are verbose.
% ...
% -void udp_ctlinput(int, struct sockaddr *, void *);
% -void udp_init(void);
% -void udp_input(struct mbuf *, int);
% -struct inpcb *udp_notify(struct inpcb *inp, int errno);
% -int udp_shutdown(struct socket *so);
% +void udp_ctlinput(int, struct sockaddr *, void *);
% +void udp_init(void);
% +void udp_input(struct mbuf *, int);
% +struct inpcb *udp_notify(struct inpcb *inp, int errno);
% +int udp_shutdown(struct socket *so);
% +
% +
% +typedef void (*udp_tunnel_func_t) (struct mbuf *, int off);
% +int udp_set_kernel_tunneling(struct socket *so, udp_tunnel_function_t f);
% +
% #endif
%
% #endif
Now everything here is a mess.
Another bug or two in indent causes it to put the bogus space after
"(*udp_tunnel_func_t)" when starting with a perfectly formatted
prototype.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20081213214209.L977>