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
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);
------------------------------
Randall Stewart
803-317-4952 (cell)
803-345-0391(direct)
_______________________________________________
freebsd-net@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to "freebsd-net-unsubscr...@freebsd.org"
