Re: Heads up --- Thinking about UDP and tunneling

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

Attachment: diff_with_s9
Description: Binary data




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)

_______________________________________________
freebsd-net@xxxxxxxxxxx mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to "freebsd-net-unsubscribe@xxxxxxxxxxx"

Relevant Pages