Date: Wed, 24 Dec 2008 23:46:12 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Randall Stewart <rrs@lakerest.net> Cc: freebsd-net <freebsd-net@freebsd.org> Subject: Re: Heads up --- Thinking about UDP and tunneling Message-ID: <20081224232356.O754@delplex.bde.org> In-Reply-To: <4A152664-B170-4C6C-85C1-58E2E1577CA3@lakerest.net> References: <D72E9703-C8E7-4A21-A71E-A4B4C2D7E8F4@lakerest.net> <494157DF.6030802@FreeBSD.org> <13C9478E-CBF6-4EDA-8E78-AD76549EB844@lakerest.net> <200812132030.15665.max@love2party.net> <4A152664-B170-4C6C-85C1-58E2E1577CA3@lakerest.net>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20081224232356.O754>