From owner-freebsd-net@FreeBSD.ORG Fri Dec 12 16:47:58 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 5025A1065670 for ; Fri, 12 Dec 2008 16:47:58 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail08.syd.optusnet.com.au (mail08.syd.optusnet.com.au [211.29.132.189]) by mx1.freebsd.org (Postfix) with ESMTP id CC1D38FC20 for ; Fri, 12 Dec 2008 16:47:57 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from c122-107-112-105.carlnfd1.nsw.optusnet.com.au (c122-107-112-105.carlnfd1.nsw.optusnet.com.au [122.107.112.105]) by mail08.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id mBCGlqxv008948 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sat, 13 Dec 2008 03:47:55 +1100 Date: Sat, 13 Dec 2008 03:47:52 +1100 (EST) From: Bruce Evans X-X-Sender: bde@delplex.bde.org To: Randall Stewart In-Reply-To: <13C9478E-CBF6-4EDA-8E78-AD76549EB844@lakerest.net> Message-ID: <20081213030449.F2405@delplex.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> 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: Fri, 12 Dec 2008 16:47:58 -0000 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