Heads up --- Thinking about UDP and tunneling

[Randall Stewart]

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..

Unless you think its already in conformance (bet you
its not) :-)

R
On Dec 12, 2008, at 11:47 AM, Bruce Evans wrote:

> 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
>

------------------------------
Randall Stewart
803-317-4952 (cell)
803-345-0391(direct)