Heads up --- Thinking about UDP and tunneling
Randall Stewart
rrs at lakerest.net
Fri Dec 12 12:27:59 PST 2008
Bruce:
So lets see:
1) I went ahead and fixed the comments.. even added a ! instead of :-(
2) No problem using func_t.. changed to that.. seems nicer :-D
3) Removed an extra cr or two you pointed out.. hopefully got them all.
4) I disagree with you on the cast... its not ugly.. it prevents us
from having to have a per_pcb structure for UDP when all we need
is one pointer. As I said in my first post.. it seemed like to much
overhead, creating a zone for a single pointer... I am not adverse
to
casts .. but of course I guess I am just an old fart who has been
around
to many years writing code :-)
5) I ran s9indent.. and of course it found a lot of other things you
missed.. but that
is the way of s9indent I have found :-D
R
-------------- next part --------------
A non-text attachment was scrubbed...
Name: diff_with_s9
Type: application/octet-stream
Size: 41349 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/freebsd-net/attachments/20081212/feceae51/diff_with_s9-0001.obj
-------------- next part --------------
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)
More information about the freebsd-net
mailing list