Date: Fri, 26 Dec 2008 06:49:51 -0500 From: Randall Stewart <rrs@lakerest.net> To: Bruce Evans <brde@optusnet.com.au> Cc: freebsd-net <freebsd-net@freebsd.org> Subject: Re: Heads up --- Thinking about UDP and tunneling Message-ID: <83B28BE4-6F66-4845-9DD6-5A9668521414@lakerest.net> In-Reply-To: <20081224232356.O754@delplex.bde.org> 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> <20081224232356.O754@delplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--Apple-Mail-146-729405199
Content-Type: text/plain;
charset=US-ASCII;
format=flowed;
delsp=yes
Content-Transfer-Encoding: 7bit
Bruce:
Ok some comments in line and an updated patch... I went
through and reverted and manually cut out the "extra's" that
s9indent (note not my script something I got for gnn) did :-)
And I also have some comments for you :-D
On Dec 24, 2008, at 7:46 AM, Bruce Evans wrote:
> 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.
I fixed this.. but I don't see anything in style(9) that talks about
taking out extra spaces.. also I find in udp6_usrreq.c (in a quick
look)
LOTS of places where an extra <cr> is present. Is this something missing
in style(9) or something recently added?
>
>
> % + if (last->inp_ppcb == NULL) {
> % + if (n != NULL)
> % + udp_append(last, ip, n, iphlen +
> % + sizeof(struct udphdr), &udp_in);
>
> Line too long.
Ok found that and did a bit of rearranging :-)
>
>
> % + 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.
Hmm.. this one must have crept back in when I took Matt's patch since
I had fixed it before. Note that just like the extra blank, I don't
see anything in style(9) that talks about punctuation in comments
(besides
the comments on the list that :-)/( and friends were thought to be
punctuation :-D)... is this too a new or just missing item from
style(9)?
>
>
> % + * % + * 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...
Ok, I dropped it down to udp_tun_func_t .. the minimum IMO
needed to make it clear what is being done. I have some of
those c++ tendency's to want things to be clear :-)
>
>
> % +
> % + tunnel_func = (udp_tunnel_func_t) last->inp_ppcb;
>
> ... line too long, caused by the verbose typedef.
I think the 3 char's fix this .. :-)
>
>
> Casts are not followed by a space in KNF. This style bug is made
> fairly
> consistently in this patch.
Hmm I wonder if that was s9indent.. need to go look at that script...
since
I normally don't put white space in front of a cast ;-D
>
>
> Bogus cast (depends on undefined behaviour to save space).
We discussed this.. and I don't think its worth adding
the 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.
Ok, All of those touches (improvements or un-improvements) are gone
now :-D
>
>
> % @@ -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).
I agree .. and have made those changes :-D
>
>
> % @@ -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).
Fixed...
>
>
> 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.
fixed both of these... but my comments above apply :-D
>
>
> % 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
>
--Apple-Mail-146-729405199
Content-Disposition: attachment;
filename=udp_patch_again.txt
Content-Type: text/plain;
x-unix-mode=0644;
name="udp_patch_again.txt"
Content-Transfer-Encoding: 7bit
Index: netinet/udp_usrreq.c
===================================================================
--- netinet/udp_usrreq.c (revision 186478)
+++ netinet/udp_usrreq.c (working copy)
@@ -488,10 +488,33 @@
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);
+ if (last->inp_ppcb == NULL) {
+ if (n != NULL)
+ udp_append(last,
+ ip, n,
+ iphlen +
+ sizeof(struct udphdr),
+ &udp_in);
+ 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.
+ *
+ * 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_tun_func_t tunnel_func;
+
+ tunnel_func = (udp_tun_func_t)last->inp_ppcb;
+ tunnel_func(n, iphlen, last);
+ INP_RUNLOCK(last);
+ }
}
last = inp;
/*
@@ -516,10 +539,24 @@
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 we must make sure
+ * all locks are released when we call the tunneling
+ * protocol.
+ */
+ udp_tun_func_t tunnel_func;
+
+ tunnel_func = (udp_tun_func_t)last->inp_ppcb;
+ tunnel_func(m, iphlen, last);
+ INP_RUNLOCK(last);
+ INP_INFO_RUNLOCK(&V_udbinfo);
+ }
return;
}
@@ -563,6 +600,18 @@
INP_RUNLOCK(inp);
goto badunlocked;
}
+ if (inp->inp_ppcb != NULL) {
+ /*
+ * Engage the tunneling protocol we must make sure all locks
+ * are released when we call the tunneling protocol.
+ */
+ udp_tun_func_t tunnel_func;
+
+ tunnel_func = (udp_tun_func_t)inp->inp_ppcb;
+ tunnel_func(m, iphlen, inp);
+ INP_RUNLOCK(inp);
+ return;
+ }
udp_append(inp, ip, m, iphlen + sizeof(struct udphdr), &udp_in);
INP_RUNLOCK(inp);
return;
@@ -1138,10 +1187,38 @@
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".
+ */
+ inp->inp_ppcb = NULL;
INP_WUNLOCK(inp);
return (0);
}
+int
+udp_set_kernel_tunneling(struct socket *so, udp_tun_func_t f)
+{
+ struct inpcb *inp;
+
+ inp = (struct inpcb *)so->so_pcb;
+ if (so->so_type != SOCK_DGRAM) {
+ /* Not UDP socket... sorry! */
+ return (ENOTSUP);
+ }
+ if (inp == NULL) {
+ /* NULL INP? */
+ return (EINVAL);
+ }
+ INP_WLOCK(inp);
+ inp->inp_ppcb = f;
+ INP_WUNLOCK(inp);
+ return (0);
+}
+
static int
udp_bind(struct socket *so, struct sockaddr *nam, struct thread *td)
{
Index: netinet/udp_var.h
===================================================================
--- netinet/udp_var.h (revision 186478)
+++ netinet/udp_var.h (working copy)
@@ -110,6 +110,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_tun_func_t)(struct mbuf *, int off, struct inpcb *);
+int udp_set_kernel_tunneling(struct socket *so, udp_tun_func_t f);
#endif
#endif
Index: netinet6/udp6_usrreq.c
===================================================================
--- netinet6/udp6_usrreq.c (revision 186478)
+++ netinet6/udp6_usrreq.c (working copy)
@@ -287,8 +287,25 @@
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 != NULL) {
+ /*
+ * 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.
+ *
+ */
+ udp_tun_func_t tunnel_func;
+
+ tunnel_func = (udp_tun_func_t)last->inp_ppcb;
+ tunnel_func(n, off, last);
+ INP_RUNLOCK(last);
+ } else {
+ udp6_append(last, n, off, &fromsa);
+ INP_RUNLOCK(last);
+ }
}
}
last = inp;
@@ -317,6 +334,19 @@
}
INP_RLOCK(last);
INP_INFO_RUNLOCK(&V_udbinfo);
+ if (last->inp_ppcb != NULL) {
+ /*
+ * Engage the tunneling protocol we must make sure
+ * all locks are released when we call the tunneling
+ * protocol.
+ */
+ udp_tun_func_t tunnel_func;
+
+ tunnel_func = (udp_tun_func_t)inp->inp_ppcb;
+ tunnel_func(m, off, last);
+ INP_RUNLOCK(last);
+ return (IPPROTO_DONE);
+ }
udp6_append(last, m, off, &fromsa);
INP_RUNLOCK(last);
return (IPPROTO_DONE);
@@ -354,6 +384,18 @@
}
INP_RLOCK(inp);
INP_INFO_RUNLOCK(&V_udbinfo);
+ if (inp->inp_ppcb != NULL) {
+ /*
+ * Engage the tunneling protocol we must make sure all locks
+ * are released when we call the tunneling protocol.
+ */
+ udp_tun_func_t tunnel_func;
+
+ tunnel_func = (udp_tun_func_t)inp->inp_ppcb;
+ tunnel_func(m, off, inp);
+ INP_RUNLOCK(inp);
+ return (IPPROTO_DONE);
+ }
udp6_append(inp, m, off, &fromsa);
INP_RUNLOCK(inp);
return (IPPROTO_DONE);
--Apple-Mail-146-729405199
Content-Type: text/plain;
charset=US-ASCII;
format=flowed
Content-Transfer-Encoding: 7bit
------------------------------
Randall Stewart
803-317-4952 (cell)
803-345-0391(direct)
--Apple-Mail-146-729405199--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?83B28BE4-6F66-4845-9DD6-5A9668521414>