Sebastien Roy wrote: > On Sat, 2009-08-01 at 09:44 -0400, James Carlson wrote: >> Sebastien Roy wrote: >>> http://www.opensolaris.org/os/project/clearview/iptun/cr/ >> 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.
What I'm asking is whether that change is ok. Unless I'm missing something, it looks like a previous IPsec fix here is being reverted. Does the previous code not matter anymore? If so, why not? >> 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. Right; I didn't think it was correct before, either. > 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? That might do it. Or all links could have addresses with some of them just reporting a zero-length address. >> 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. We do something similar for I_POP, so it seems at least feasible. A crude answer would just be to insert a check for these special names in the kernel's _I_REMOVE and deny it there. (I was commenting on enforcing kernel design rules like this out in user space. It doesn't make sense to me.) >> 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? OK, I see; it relies on the exit from dladmerr_exit. >> 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. That's might be true with some tunnel-mode IPsec configurations, but I'm not so sure it's generally true. I'd expect that you can have essentially arbitrary IPv6 headers between the outer and inner tunnel headers. (Unless the kernel is doing something here to rewrite these headers to remove things the user "shouldn't" see ...) >> 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". OK. >> 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. OK. >> dladm.c >> 979: seems stray. > > These are used to print iptun_flags in print_iptun(). OK ... but they don't look related to any of the column handling, so having this enum pasted right here looks a little odd. >> 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. Agreed; the style in this file is awful. But if you're going to pick a new and better style, I think you've got a bit more lifting to do. ;-} >> 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. Strange ... >> 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? You and meem can have a replay of the same argument I had with him. I agree with you, and if it were my code, I wouldn't use this ".xcl" thing, but meem finds the gettext() wrappers everywhere to be more offensive for some reason, so that's why a lot of this code needs to be maintained this way. >> 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 OK. Really, the compilers should just be updated to produce CTF so we can ditch this hackery. >> 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. I thought we were using a more restricted set for datalinks on purpose ... but ok. Does this mean that '.' is intentionally allowed for non-tunnel vanity names? So I can use "foo.bar.baz0" if I want, and it doesn't imply driver foo with modules bar and baz? Is this something that will be documented? >> 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. There's a lot in that area that's mysterious to me. >> 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. OK; just checking. -- James Carlson 42.703N 71.076W <[email protected]> _______________________________________________ networking-discuss mailing list [email protected]
