Date: Wed, 17 Nov 1999 03:26:55 +0900 From: Yoshinobu Inoue <shin@nd.net.fujitsu.co.jp> To: pb@fasterix.freenix.org Cc: sakane@kame.net Subject: Re: [Solicite review for KAME 2nd patch](netinet6 basic part addtion) Message-ID: <19991117032655J.shin@nd.net.fujitsu.co.jp> In-Reply-To: <19991112152848.A90019@fasterix.frmug.org> References: <19991112115745B.shin@nd.net.fujitsu.co.jp> <19991112152848.A90019@fasterix.frmug.org> <19991113012530.A62524@fasterix.frmug.org>
next in thread | previous in thread | raw e-mail | index | archive | help
# This mail is a reply to the comment for my 2nd patch of KAME # merging into freebsd-current. # I cc this also to freebsd-arch, and would like the patch also # to be reviewed by freebsd-arch members. # Patches are placed at # http://paradise.kame.net/v6proxy/diana2/shin/work/freebsd/kame-into-fbc.html # Thanks in advance! > > Please give me comments. > > Here are some relatively minor comments where I feel like I have > something interesting to say. I haven't taken the time to read the > patches in detail yet, I'll do that later. Thanks for your comments and patches! > > (1)KAME added new files sys/net/net_osdep.{c,h} for source code > > compatibility between different BSDs. > > Several KAME files depends on them. > > Could I just add those files to FreeBSD-current? > > Given that sys/net/*.h is supposed to end up in /usr/include/net, > IMHO a better way to handle that stuff would be to insert the > appropriate defines in the relevant include files, with associated > comment that it should be kept for compatibility purposes with > other BSDs. That would have the interesting side-effect of being > "more" compatible with other BSDs without even having to include > some special file. > > For example ifa_list should be defined where ifa_link is, i.e. > net/if_var.h. I moved them. > > (3)struct inpcb in sys/netinet/in_pcb.h is shared between > > netinet and netinet6, and much change is done on it. > > There are some needless tab <-> space diffs there, in inp_hash, > inp_list and inp_portlist for example. Also, I think the structure > had been carefully optimized so that the most accessed fields are > in the first 16 bytes (a i386-cache optimization I suppose), it > would be nice to keep that if possible. I mistakenly thought that a tab is also necessary after queue macro member as well as after var type or struct. I think I fixed all of them. About in_pcb first 16 bytes, I tried to re-order each members in my new patch, but addr part is difficult because it is now shared with IPv6 addr, and anyway, IPv6 addrs can not be put into 16 bytes. Should I add new pointer members which point to actual addr part and put them into first 16 bytes, or is it meaningless? > A followup on my first comments. > > 1) regarding the PF_KEY stuff, you define several extensions > for crypto algorithms not in RFC 2367, using SADB_EALG_ as a > prefix instead of SADB_EALG_X_ as requested by the RFC. > > Furthermore, the names you chose seem to be different from names > in OpenBSD (2.5) headers. > > (see http://www.openbsd.org/cgi-bin/cvsweb/src/sys/net/pfkeyv2.h?rev=1.12) > > OpenBSD uses these, I think it would make sense to take the same > conventions (unless there's other in-progress RFC work for this > on which you based your code, of course): > > #define SADB_X_EALG_BLF 3 > #define SADB_X_EALG_CAST 4 > > Same remark for keyed MD5 and SHA1, OpenBSD uses these: > #define SADB_X_AALG_MD5 6 > #define SADB_X_AALG_SHA1 7 About this, I had a discussion with an actual implementer of that part. The part of KAME code you specified is based on a old draft of RFC2367 and he know it doesn't confrom to RFC2367. But real problem is that RFC2367 is not complete in many part, and each IPsec implementations(including KAME) has its own extensions(not only above part), and are not portable each other, anyway. He think updating current pfkey header as RFC2367 is meaingless, and wish to update it based on some commonly agreed new pfkey spec, and wish to just keep it as is for now. > 2) rather than having net/pfkeyv2.h an empty include calling > netkey/keyv2.h, wouldn't it be more sensible to do the opposite? > As I understand it, net/pfkeyv2.h is the "standard" name from > RFC 2367. It is mainly historical reason. Now it seems to be best timing to change the file to correct place, so I moved netkey/keyv2.h to net/pfkeyv2.h. > 3) there are many places with #ifdef code for NetBSD or OpenBSD; > couldn't at least some of these be removed? > > 4) related question for #ifdef FreeBSD: shouldn't the #ifdef go > away? I removed many #ifdef's including them, which seemed to be removable. > 5) a nitpick regarding indentation, there are a _lot_ of places where > "else" is indented like this, which is not allowed by style(9): > } > else { > > This should rather be: > } else { > > 6) another nitpick, there are many places with a needless trailing > space. > > I'm sending you a patch to your patch (!) to correct some of 5) > and 6). This is just intended as a manual reference for you, as I > made it by hand so it breaks the line numbers in your diffs. Thanks a lot! I removed unnecessary white spaces, and fixed indentations. Also I noticed several INET6s in header files, so I removed them as much as possible. New patches are placed below. http://paradise.kame.net/v6proxy/diana2/shin/work/freebsd/kame-into-fbc.html >> By the way, should this kind of review request be sent to >> freebsd-current, usually? >Yes. Actually freebsd-arch would probably be the best place for this. >You might consider reposting this there and asking for followups to be >discussed there. This time, I added cc to freebsd-arch as commented at the top. Thanks for all the comments again. Yoshinobu Inoue 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?19991117032655J.shin>