Date: Sat, 4 Dec 1999 15:48:07 +0100 From: Jeroen Ruigrok/Asmodai <asmodai@wxs.nl> To: Yoshinobu Inoue <shin@nd.net.fujitsu.co.jp> Cc: freebsd-arch@freebsd.org, cvs-committers@freebsd.org Subject: Re: [Solicite review for KAME 3rd patch] Message-ID: <19991204154807.G711@daemon.ninth-circle.org> In-Reply-To: <19991203050711F.shin@nd.net.fujitsu.co.jp>; from shin@nd.net.fujitsu.co.jp on Fri, Dec 03, 1999 at 05:07:11AM %2B0900 References: <19991203050711F.shin@nd.net.fujitsu.co.jp>
next in thread | previous in thread | raw e-mail | index | archive | help
Hello Inoue-san,
-On [19991203 00:00], Yoshinobu Inoue (shin@nd.net.fujitsu.co.jp) wrote:
>I prepared the 3rd KAME patch at the URL below, and would like
>to get comments for them.
Here are my comments:
[NOTE: all based on a clean extracted src from 09:00 GMT+1 cvsup
sources]
All patches succeeded on a fresh CURRENT source tree except for:
- sbin/ifconfig/Makefile
no .rej file was made curiously enough. Patched by hand.
- sbin/route/Makefile
no .rej file was made. Patched by hand.
- usr.bin/netstat/Makefile
also no .rej file. Patched by hand.
sys/conf/files:
I like the rearranging of the netinet options, it certainly clarifies
things.
sys/i386/conf/LINT:
COMPATIBILITY OPTIONS seems to be a whitespace fix. Not needed IMHO.
Does the gif comment imply that IPv4 in IPv4, IPv4 in IPv6, IPv6 in
IPv4, IPv6 in IPv6 tunneling is supported? I think it does, but I
wonder if others might not understand the notation used. One more
comment line to clarify things from the start wouldn't hurt and would
save questions to -questions. Also, use `gif' instead of `gif`, minor
nitpick to remain consistent.
The `faith` pseudo-device catches those packets which sent to it, and
forwards then to IPv4 and IPv6 translating daemon.
Should this be:
The `faith' pseudo-device captures packets sent to it and forwards them
to the IPv4/IPv6 translation daemon.
Although I wonder if forwards could be changed to divert.
Something which I wonder about is the removal of opt_inet.h in if.c.
All other source files seem to retain their opt_inet.h alongside the
opt_inet6.h. Was this intentional?
Does faith replace loop? If so, what is against adding the IPv6
functionality to loop?
if_fddisubr.c: contains a whitespace fix for netipx/ipx.h, not needed
IMHO. Also contains whitespace fixes for llc_snap_*. Not needed.
Same goes for senderr(), the else statement, the empty line `fix',
register struct sockaddr_dl *sdl, the printf(" len 0x%x, the #endif, the
#ifdef IPX and following #endif, #ifdef NETATALK, case LLC_ISO_LSAP, the
other empty line `fix', fh->fddi_dhost, and the last empty line `fix'.
Also, wouldn't it be better to follow INET6 directly after INET, since
both are effectively the same protocol, except a different version?
Just like you did with the #ifdef INET6 later on in this file. Later on
with the ETHERTYPE cases, you now position yourself between NS and
DECNET, why not follow directly after INET?
if_gif.h: Numerous white space changes which are not needed and
sometimes even revert the previous style(9) compliant code. Removal of
the $FreeBSD$ doesn't seem needed.
if_spppsubr.c: #define MAXALIVECNT is a whitespace change, the #define
whitespace changes are not needed. Same goes for the */ `fix'. And
another empty line `fix'. The IFF_CISCO whitespace fixes are also not
needed. Some more #define and empty line whitespace `fixes'.
if_atm.c: No need to touch the disclaimer for a whitespace fix. And
aside fine the new #include does only whitespace fixes which are not
needed.
in_pcb.c: More tab/whitespace `fixes' aside from the new includes
added and the #ifdef INET6's.
in_pcb.h: Again, aside from one change, you shouldn't do whitespace
fixes.
in_proto.c: again a whitespace fix which shouldn't be in there.
ip_fw.c: aside from changed function calls and the likes, more
whitespace fixes.
tcp_input.c: suffer from whitespace fixes.
And the file suffers from continuous whitespace fixes. Find an update
of your patch at http://home.wxs.nl/~asmodai/udp-for-ipv6-fixed.19991203 This
update only fixes the whitespace problems for a couple files. I grew weary
after file 16 or so.
I compiled a world and got:
cc -O -pipe -DKERNEL -Wall -Wredundant-decls -Wnested-externs -Wstrict-prototypes -Wmissing-prototypes -Wpointer-arith -Winline - Wcast-qual -fformat-extensions -ansi -DKLD_MODULE -nostdinc -I- -I/usr/obj/usr/src/sys/modules/if_disc -I/usr/obj/usr/src/sys/modules/if_disc/@ -mpreferred-stack-boundary=2 -c /usr/src/sys/modules/if_disc/../../net/if_disc.c /usr/src/sys/modules/if_disc/../../net/if_disc.c:55: opt_inet6.h: No such file or directory
*** Error code 1
Stop in /usr/src/sys/modules/if_disc.
My suggestion: checkout a new source tree and only modify that what you want to
change and ignore tab line-ups and concentrate on the functionality. White
space corrections are best done in cosmestic commits.
Hope this helps,
--
Jeroen Ruigrok van der Werven/Asmodai asmodai(at)wxs.nl
The BSD Programmer's Documentation Project <http://home.wxs.nl/~asmodai>;
Network/Security Specialist BSD: Technical excellence at its best
Learn e-mail netiquette: http://www.lemis.com/email.html
...fools rush in where Daemons fear to tread.
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-arch" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?19991204154807.G711>