Heads up --- Thinking about UDP and tunneling

[Bruce Evans]

On Tue, 23 Dec 2008, Randall Stewart wrote:

> 4) revamped my s9indent use.. I ran it and then patched back
>   in just its complaints about me... that way the other s9 issues
>   can stay in the file untouched by me :-D

Thanks, but it still has many of the style bugs already pointed out
and a few new ones.  Please fix them and s9indent so that they don't
get pointed out again :-).

% Index: netinet/udp_usrreq.c
% ===================================================================
% --- netinet/udp_usrreq.c	(revision 185928)
% +++ netinet/udp_usrreq.c	(working copy)
% @@ -488,41 +488,76 @@
%  				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 we
% +					 * will have to leave the info_lock
% +					 * up, since we are hunting through
% +					 * multiple UDP inp's hope we don't
% +					 * break :-(

Missing punctuation.

% +					 * 
% +					 * XXXML: Maybe add a flag to the
% +					 * prototype so that the tunneling
% +					 * can defer work that can't be done
% +					 * under the info lock?
% +					 */
% +					udp_tunnel_func_t tunnel_func;

This typedef is very verbose...

% +
% +					tunnel_func = (udp_tunnel_func_t) last->inp_ppcb;

... line too long, caused by the verbose typedef.

Casts are not followed by a space in KNF.  This style bug is made fairly
consistently in this patch.

Bogus cast (depends on undefined behaviour to save space).

% +					tunnel_func(m, iphlen, last);
% +					INP_RUNLOCK(last);
% +				}
%  			}
%  			last = inp;
%  			/*
% -			 * Don't look for additional matches if this one does
% -			 * not have either the SO_REUSEPORT or SO_REUSEADDR
% -			 * socket options set.  This heuristic avoids
% -			 * searching through all pcbs in the common case of a
% -			 * non-shared port.  It assumes that an application
% -			 * will never clear these options after setting them.
% +			 * Don't look for additional matches if this one
% +			 * does not have either the SO_REUSEPORT or
% +			 * SO_REUSEADDR socket options set.  This heuristic
% +			 * avoids searching through all pcbs in the common
% +			 * case of a non-shared port.  It assumes that an
% +			 * application will never clear these options after
% +			 * setting them.
%  			 */
%  			if ((last->inp_socket->so_options &
% -			    (SO_REUSEPORT|SO_REUSEADDR)) == 0)
% +			    (SO_REUSEPORT | SO_REUSEADDR)) == 0)

You didn't have to touch this statement.  Touching it improves it.

%  				break;
%  		}
% 
%  		if (last == NULL) {
%  			/*
% -			 * No matching pcb found; discard datagram.  (No need
% -			 * to send an ICMP Port Unreachable for a broadcast
% -			 * or multicast datgram.)
% +			 * No matching pcb found; discard datagram.  (No
% +			 * need to send an ICMP Port Unreachable for a
% +			 * broadcast or multicast datgram.)

You didn't have to touch this.  Touching it unimproves it.

% ...
%  	}
% -
%  	/*
%  	 * Locate pcb for datagram.
%  	 */
% @@ -553,7 +588,6 @@
%  		INP_INFO_RUNLOCK(&V_udbinfo);
%  		return;
%  	}
% -
%  	/*
%  	 * Check the minimum TTL for socket.
%  	 */

You didn't have to touch these.  Touching them arguably unimproves them,
though strict KNF probably doesn't permit these blank lines.

% @@ -563,6 +597,18 @@
%  		INP_RUNLOCK(inp);
%  		goto badunlocked;
%  	}
% +	if (inp->inp_ppcb) {

It is preferable to compare pointers explicitly with NULL.  The previous
comparision of inp_ppcb in this patch is explicit (but it is for equality).
This and all of the following comparisions are implicit (for inequality).

% @@ -1138,10 +1184,41 @@
%  	INP_INFO_WUNLOCK(&V_udbinfo);
%  	inp->inp_vflag |= INP_IPV4;
%  	inp->inp_ip_ttl = V_ip_defttl;
% +	/*
% +	 * UDP does not have a per-protocol
% +	 * pcb (inp->inp_ppcb). We use this
% +	 * pointer for kernel tunneling pointer.
% +	 * If we ever need to have a protocol
% +	 * block we will need to move this
% +	 * function pointer there. Null
% +	 * in this pointer means "do the normal
% +	 * thing".
% +	 */

Lines too short (formatted for 50-column terminals).

Normal entence breaks are 2 spaces, not 1 as in this comment.  Most of the
other comments in this patch have normal sentence breakes.

% ...
% +int
% +udp_set_kernel_tunneling(struct socket *so, udp_tunnel_func_t f)
% +{
% +	struct inpcb *inp;

Missing blank line after declarations.

% +	inp = (struct inpcb *)so->so_pcb;
% +

Extra blank line.

% +	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_func_t)(struct mbuf *, int off, struct inpcb *);
% +int udp_set_kernel_tunneling(struct socket *so, udp_tunnel_func_t f);
%  #endif
% 
%  #endif

Still has a style bug density of about 5 per line :-(.

% Index: netinet6/udp6_usrreq.c

Similarly.

Bruce