Sebastien Roy wrote:
> http://www.opensolaris.org/os/project/clearview/iptun/cr/
I read through all of SFW (not much there) and most in ON. I glazed
over on dlmgmtd and a fair bit of the kernel changes.
ifconfig.c:
What happened to the tunnel reality check (IPsec thing)?
1614: tunnels aren't the only things that lack Ethernet addresses.
There should probably be another way to do this.
2424: this looks like a test that belongs in the kernel.
2476-2477: instead of littering this test everywhere, consider
having it in dladm_link_open (combination of ':' and
DATALINK_CLASS_IPTUN is bad) or dladm_iptun_modify, with an
appropriate error code.
2536: this will do a close even if the open fails, unlike the very
similar code at 2482.
3015: lost the "tabbed" flag, which avoided spurious blank lines.
It should be added back in. (And consider putchar('\n'); instead
of printf("\n");.)
3909: s/is no assigned/is not assigned/
4178: this makes the bug at 2536 much more serious; a wrong link
type means that the handle will be closed twice!
4186: missing blank line.
snoop_ether.c:
1704: what happens with IPsec or labeling?
1713-1725: this seems like a lot of work. Isn't the IP packet
just nested inside the tunnel? Why allocate a new buffer and
copy over? Why not just index out to the IP header start?
snoop_filter.c:
1382: except for two constants, this looks like a duplicate
table. It'd be nice if there were a better way.
snoop_ip.c:
370, et cetera: as long as you're tidying up here, this code
can't lint cleanly. You might as well fix that. (Or maybe
just leave it alone ...)
dladm.c:
355: table lacks "-R"/"--root-dir" option.
365: the array itself should also be const.
979: seems stray.
987: could also be const.
988: why not follow the style used elsewhere for these tables?
2200, et cetera: suggest strcpy instead of sprintf.
2222: this doesn't belong here. (See 2233.)
2253: why is this the only type that now returns errors? You've
fixed it so that all other cases result in "?" in the affected
field, why not this one as well? (And if nothing returns errors
anymore, why not make this function "void"?)
2583: why bother testing?
2964: code standard: operator belongs on previous line.
3404: 'char' isn't guaranteed to be signed. This needs to be 'int'
to match the getopt() return type.
3571: doesn't appear to be used. (Got lint?)
dladm.xcl:
Check that your getopt strings aren't showing up in the text for
translation. (The additions here seem sparse.)
dlmgmtd/Makefile:
46: broader issue: why isn't CTF just the default?
dlmgmt_db.c:
193: careful with this: any lazy-loading libraries that the invoked
function uses will also be loaded from the not-quite-trusted
non-global zone. (And if anything is on NFS, then all bets are off.)
May need to consult with the Trusted Extensions folks on this.
network-initial.xml:
Removing the crypto dependency sounds right, but just checking whether
the security guys have looked at this. (Can anything other than a
tunnel require crypto?)
native/zone/config.xml:
Where does ipkg/config.xml come from, and does it need to be updated
as well?
libdladm.c:
776: is this true for all links now?
libdlaggr.c:
359: test not necessary.
libdladm:
May need to check the translation tables and .xcl file. New strings
seem to have been added to the library and some are tokens that should
not be translated.
minorperm:
There's a new iptun.conf but no minorperm entry for iptun.
ip_multi.c:
1391: tested with multicast over PPP?
netstack.h:
62: s/done ine in/done in/
--
James Carlson 42.703N 71.076W <carlsonj at workingcode.com>
