Date: Sat, 13 Dec 2008 03:47:52 +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: <20081213030449.F2405@delplex.bde.org> In-Reply-To: <13C9478E-CBF6-4EDA-8E78-AD76549EB844@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>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, 12 Dec 2008, Randall Stewart wrote:
> Here is an updated patch it:
>
> 1) Fixes style9 issues (I hope.. I went back to vi and tried tabs :-0.. sigh
> one of
> these doys I will figure out why my .emacs settings just never cut it :-()
Fraid not.
% Index: netinet/udp_usrreq.c
% ===================================================================
% --- netinet/udp_usrreq.c (revision 185928)
% +++ netinet/udp_usrreq.c (working copy)
% @@ -488,10 +488,25 @@
% 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);
% +
Extra blank line.
% + if (last->inp_ppcb == NULL) {
% + if (n != NULL)
% + udp_append(last, ip, n, iphlen +
% + sizeof(struct udphdr), &udp_in);
Line too long.
% + INP_RUNLOCK(last);
% + } else {
% + /* Engage the tunneling protocol
"/*" not on a line by itself.
% + * we will have to leave the info_lock
% + * up, since we are hunting through
% + * multiple UDP inp's hope we don't break :-(
Line too long. Defeats reasonable indentation of the rest of the comment.
Missing punctuation after ":-(".
% + */
% + udp_tunnel_function_t tunnel_func;
Nested declaration.
% +
% + INP_RUNLOCK(last);
% + tunnel_func = (udp_tunnel_function_t)last->inp_ppcb;
Line too long.
Typedef names involving functions normally use `func_t', not `function_t'.
This is useful for reducing verboseness and resulting long lines but wouldn't
fix the long line in the above, since everything in it is too verbose
(in the middle there is an ugly cast whose only effect can be to avoid a
warning ...).
% @@ -516,10 +531,25 @@
% V_udpstat.udps_noportbcast++;
% goto badheadlocked;
% }
% - udp_append(last, ip, m, iphlen + sizeof(struct udphdr),
% - &udp_in);
% - INP_RUNLOCK(last);
% - INP_INFO_RUNLOCK(&V_udbinfo);
% + if (last->inp_ppcb == NULL) {
% + udp_append(last, ip, m, iphlen + sizeof(struct udphdr),
% + &udp_in);
% + INP_RUNLOCK(last);
% + INP_INFO_RUNLOCK(&V_udbinfo);
% + } else {
% + /* Engage the tunneling protocol
"/*" not on a line by itself. No comment on further instances of this
% + * we must make sure all locks
% + * are released when we call the
% + * tunneling protocol.
% + */
Long lines are ones longer than 80 characters. Splitting at 48 characters
as in the above is not normal.
% + udp_tunnel_function_t tunnel_func;
Nested declaration.
% @@ -563,6 +593,18 @@
% INP_RUNLOCK(inp);
% goto badunlocked;
% }
% + if (inp->inp_ppcb) {
% + /* Engage the tunneling protocol
% + * we must make sure all locks
% + * are released when we call the
% + * tunneling protocol.
% + */
More formatting for 48-column terminals.
% + udp_tunnel_function_t tunnel_func;
Nested declaration.
Missing blank line after declarations.
% ...
% +int
% +udp_set_kernel_tunneling(struct socket *so, udp_tunnel_function_t f)
% +{
% + struct inpcb *inp;
% + inp = (struct inpcb *)so->so_pcb;
Initialization in declaration. Not too bad here, but you don't do it for
similar tunnel function pointer conversions.
% +
% + if (so->so_type != SOCK_DGRAM) {
% + /* Not UDP socket... sorry */
Missing punctuation.
% Index: netinet/udp_var.h
% ===================================================================
% --- netinet/udp_var.h (revision 185928)
% +++ netinet/udp_var.h (working copy)
% @@ -107,6 +107,10 @@
% 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_function_t)(struct mbuf *, int off);
% +int udp_set_kernel_tunneling(struct socket *so, udp_tunnel_function_t f);
I noticed this first in the initial patch. It has a style bug density of
about 5 per line !-(:
- 2 extra blank lines
- typedef in the middle of (non-pointer non-typedef) prototypes
- unsorted prototypes (at least the old 3 visible on the above are sorted)
- new prototypes not indented normally though visible old ones all are
- some parameters have names, some not.
- style(9) says to always have names in the kernel, but this rule is usually
violated
- the first of the 3 visible old prototypes violates this rule
- the first of the new prototypes violates this rule for the first of its
2 parameters only
% #endif
%
% #endif
% Index: netinet6/udp6_usrreq.c
% ===================================================================
% --- netinet6/udp6_usrreq.c (revision 185928)
% +++ netinet6/udp6_usrreq.c (working copy)
% @@ -286,9 +286,21 @@
% struct mbuf *n;
%
% if ((n = m_copy(m, 0, M_COPYALL)) != NULL) {
% - INP_RLOCK(last);
% - udp6_append(last, n, off, &fromsa);
% - INP_RUNLOCK(last);
% + if (last->inp_ppcb) {
% + /* Engage the tunneling protocol
% + * we will have to leave the info_lock
% + * up, since we are hunting through
% + * multiple UDP inp's hope we don't break :-(
% + */
Lines too long -- now formatting for 94-column terminals instead of
48-column ones using cut&pasted&indent.
Missing punctuation (cut&paste).
% + udp_tunnel_function_t tunnel_func;
Nested declaration.
Line too long.
Missing blank line after declarations.
% + tunnel_func = (udp_tunnel_function_t)last->inp_ppcb;
Line too long.
% + INP_RUNLOCK(last);
% + tunnel_func(m, off);
% + } else {
% + INP_RLOCK(last);
% + udp6_append(last, n, off, &fromsa);
Line too long.
% @@ -317,6 +329,19 @@
% }
% INP_RLOCK(last);
% INP_INFO_RUNLOCK(&V_udbinfo);
% + if (last->inp_ppcb) {
% + /* Engage the tunneling protocol
% + * we must make sure all locks
% + * are released when we call the
% + * tunneling protocol.
% + */
Now formatting for 56-column terminals.
% @@ -354,6 +379,18 @@
% }
% INP_RLOCK(inp);
% INP_INFO_RUNLOCK(&V_udbinfo);
% + if (inp->inp_ppcb) {
% + /* Engage the tunneling protocol
% + * we must make sure all locks
% + * are released when we call the
% + * tunneling protocol.
% + */
Back to formatting for 48-column terminals.
There are 6 near-copies of this comment.
% + udp_tunnel_function_t tunnel_func;
Nested declaration.
Missing blank line after declaration.
% + tunnel_func = (udp_tunnel_function_t)inp->inp_ppcb;
% + INP_RUNLOCK(inp);
% + tunnel_func(m, off);
Do you have to hold the lock to access inp->inp_ppcb? Otherwise you can
avoid all the nested declarations and just use the pointer once. If the
lock is needed, then what happens if the pointer changes after it is
accessed?
% + return (IPPROTO_DONE);
% + }
% udp6_append(inp, m, off, &fromsa);
% INP_RUNLOCK(inp);
% return (IPPROTO_DONE);
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20081213030449.F2405>