On Sat, 2009-08-01 at 09:44 -0400, James Carlson wrote:
> 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.
Thanks very much!
>
> ifconfig.c:
>
> What happened to the tunnel reality check (IPsec thing)?
Ifconfig no longer outputs or verifies specific policy associated with
the underlying link. If there's policy associated with the tunnel, it
refers the caller to ipsecconf.
> 1614: tunnels aren't the only things that lack Ethernet addresses.
> There should probably be another way to do this.
That's true, there's also an escape clause for IFF_VIRTUAL and IFF_IPMP
interfaces a few lines above. Note that I'm not changing the logic of
the code here, but merely calling a libdladm function instead of a
libinetcfg one to determine if the underlying link is a tunnel.
Are you suggesting that there be a link property that allows one to
derive how to print addresses associated with a given link? Or
something else?
> 2424: this looks like a test that belongs in the kernel.
It looks like it should be, but can there be such a check? I don't know
enough about how _I_REMOVE works to know if it's even possible to
prevent such an operation from the module being removed. If it's
possible, the perhaps a bug should be filed.
> 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.
ACCEPT: dladm_iptun_modify() takes a datalink_id_t, so it never comes
close to handling any IP interface names. Doing the check in
dladm_link_open() is a good idea. I'll do that.
> 2536: this will do a close even if the open fails, unlike the very
> similar code at 2482.
ACCEPT
> 3015: lost the "tabbed" flag, which avoided spurious blank lines.
> It should be added back in. (And consider putchar('\n'); instead
> of printf("\n");.)
ACCEPT: I never noticed this because the hoplimit property always exists
for all tunnel types, so there's always something to print a '\n' after.
I'll add this check back in, although I'll rename it ("tabbed" doesn't
make any sense).
>
> 3909: s/is no assigned/is not assigned/
ACCEPT
>
> 4178: this makes the bug at 2536 much more serious; a wrong link
> type means that the handle will be closed twice!
There's no bug here that I can see. If dladm_link_open() doesn't return
DLADM_STATUS_OK, then the handle isn't open, and it shouldn't be closed
by the caller. You're reiterating your comment at 2536, right?
> 4186: missing blank line.
ACCEPT
>
> snoop_ether.c:
>
> 1704: what happens with IPsec or labeling?
There are no IPsec headers here. Promiscuous streams get their packets
from GLDv3. On receive, IPsec processing occurs before packets are
passed up to GLDv3. On transmit IPsec processing occurs after packets
have been send down by GLDv3.
ACCEPT the part regarding labeling, it looks like that won't work
indeed. I'll fix this code to skip any extension headers (not just
those associated with TX labels).
> 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?
ACCEPT: Setting the global "data" pointer to a buffer containing the
entire packet is something that interpret_ether() does, and I assumed
that there was a reason for it. Looking further into other
interpret_*() functions, it doesn't look like that's needed. We should
only need to allocate and copy if the original buffer isn't sized
appropriately.
> snoop_filter.c:
>
> 1382: except for two constants, this looks like a duplicate
> table. It'd be nice if there were a better way.
DEFER: I've filed 6867865. I can't say that I'm very enthusiastic about
making snoop*.c any better though. My attitude when writing this code
was, "make this work given the existing constraints of the garbage in
here".
>
> 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 ...)
REJECT: I have to change this file up at 127 so that we print the
inner-most IP addresses, and not the outer-most. I'm changing enough so
that it's cstyle clean, but I'm not going to go through and make this
lint clean.
>
> dladm.c:
>
> 355: table lacks "-R"/"--root-dir" option.
ACCEPT
>
> 365: the array itself should also be const.
ACCEPT
>
> 979: seems stray.
These are used to print iptun_flags in print_iptun().
>
> 987: could also be const.
ACCEPT
>
> 988: why not follow the style used elsewhere for these tables?
Because I found them illegible anyway. :-} I suppose I'll fix this to
be consistently illegible.
>
> 2200, et cetera: suggest strcpy instead of sprintf.
ACCEPT
>
> 2222: this doesn't belong here. (See 2233.)
ACCEPT
> 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"?)
ACCEPT: Because Rishi added this code after I had modified the other
classes, and I neglected to fix the simnet handling here after merging.
Will fix.
>
> 2583: why bother testing?
I don't know, this was there before. I can probably be removed...
>
> 2964: code standard: operator belongs on previous line.
ACCEPT
>
> 3404: 'char' isn't guaranteed to be signed. This needs to be 'int'
> to match the getopt() return type.
ACCEPT
>
> 3571: doesn't appear to be used. (Got lint?)
ACCEPT: Hmm, lint isn't complaining about this one. All of this code is
lint free relative so the SS12 lint command. Will fix.
>
> dladm.xcl:
>
> Check that your getopt strings aren't showing up in the text for
> translation. (The additions here seem sparse.)
ACCEPT: The architecture around this facility looks way out of hand and
unmaintainable. Shouldn't it be implied that if it's not wrapped around
in a gettext(), then it doesn't need translating?
>
> dlmgmtd/Makefile:
>
> 46: broader issue: why isn't CTF just the default?
There are existing CRs on this subject:
6811460 daemons should get CTF data
5062878 ctf data should be available for important system daemons
>
> 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.)
Right, but the idea is that because the fork'ed child has entered the
zone, any such lazy-loading of garbage into that process can't hurt
anything outside of the zone in question.
> May need to consult with the Trusted Extensions folks on this.
Okay, will do. I've already consulted with the zones team (who were the
ones who actually suggested this approach).
>
> 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?)
ACCEPT: will make sure danmcd looks at the changes in this file.
>
> native/zone/config.xml:
>
> Where does ipkg/config.xml come from, and does it need to be updated
> as well?
ACCEPT: It comes from the IPS OpenSolaris project, and the sourc resides
at:
http://src.opensolaris.org/source/xref/pkg/gate/src/brand/
I believe Dan Price maintains it. Yes, it will need to be updated.
>
> libdladm.c:
>
> 776: is this true for all links now?
Yes. Becuase '.' has been allowed in IP interface names since at least
Solaris 8, there is little chance that this will cause any problems.
>
> libdlaggr.c:
>
> 359: test not necessary.
ACCEPT
>
> 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.
ACCEPT
>
> minorperm:
>
> There's a new iptun.conf but no minorperm entry for iptun.
ACCEPT: Curiously, the lack of a minor_perm entry for iptun has not
prevented /dev/net nodes from having the correct 666 permissions.
>
> ip_multi.c:
>
> 1391: tested with multicast over PPP?
ACCEPT: Will test. Since sppp doesn't handle DL_ENABMULTI_REQ and
DL_DISABMULTI_REQ, I don't think this will be problematic.
>
> netstack.h:
>
> 62: s/done ine in/done in/
ACCEPT
Thanks,
-Seb
