responses to Dave's code review comments follow... > ndp.xml: > > 34: I think this should be ndp, not in.ndpd; at least that's the > convention in a couple of other manifests I checked. This comment also > applies to the other manifests.
accepted. > 67: After looking at svc-ndp, why isn't ":kill" sufficient? And there > is certainly no reason to pass the arguments. > > > 77: Similarly, ":kill -HUP" seems like it would do the job. yep, :kill and :kill -HUP are indeed sufficient, i'll make the the changes. > 92: please say "group" instead of "pg"; would not "reserved" be more > correct than "accessible"? This comment also applies to the other > manifests. accepted. > 105: perhaps "neighbor discovery" would be better than "autoconfiguration"? sure. > svc-ndp: > > This whole file would be superfluous if SMF provided a way to tag > services as global-zone-only; I believe this RFE exists already, but if > not, please file it; at least add a comment noting it would be useful > for these services, too. Anyway, related to my comments above, you only > need the "start" portion here, the rest would be replaced by the > contract signal functionality. couldn't find a similar bug/RFE, as a result i've filed: CR6431298: smf could provide a way of tying exec methods to global/non-global zones and added you to the interest list. > svc-ripng: > > 42: So why isn't it better to replace the /etc/.dynamic_routing file > used here and in other method scripts with a boolean property on the > network/routing service? it's much better - i've made this change and excised references to .dynamic_routing as appropriate, replacing the file with a boolean dynamic_routing application property in network/routing which is set to true by the start methods of routing daemons (it's set to false in network/routing if global routing services ipv4(6)-routing are both disabled, since this ensures no routing daemons can run). > in.rdisc.c: this file shouldn't have been changed at all. > it was just moved, i updated the copyright info since wx pbchk was complaining. maybe this is wx ignorance on my part - it still needs to be in the active list due to the move, right? > routing.xml: could you add comments as to the reason for each dependency? sure. > svc-routeadm: > > 44: this whole nawk script is sufficient evidence that routeadm -p ought > to be enhanced. At the very least please file an RFE to add > functionality sufficient to eliminate this, if you don't have the > ability to add it in this project. good idea - how about supporting: # routeadm -p current/option_name # routeadm -p default/option_name # routeadm -p option_name (default gives current option value) in addition to routeadm -p with no arguments? this may require a PSARC fasttrack, i'll look into it. > 82: wouldn't it be advisable to use "cp -p" here? > 90: s/invokation/invocation/ both accepted. > 311: why do we need a pid file? and is /var/tmp the best place for it? i was following legacy routeadm practice here - it monitored the running state of such daemons using pidfiles in /var/tmp. would /tmp be better, to ensure pidfiles don't hang around post-reboot? > net-init: > > 32-37: I'd suggest just referencing the service name, not the script > name, in these comments; yeah, I know you're following what's there. It > was wrong. sure. > network-initial.xml: > > 68: I don't think the dependency is necessary - see comments later about > seed. accepted. > net-loopback: > > 52: I don't get why the service enables that are done in net-routing > based on this aren't just put here. if i can remember i was trying to keep all routing-related configuration in network/routing, though introducing another /etc file is a pretty lame way of doing it. also, thinking about it i suspect that on first boot, since manifest-import might not have run at this point, these service state changes would fail (whether they were made by an in-seed network/routing service or by network/loopback). maybe the right answer here is adding an initialize_routing property to network/loopback that network/routing (no longer in the seed as you suggest below) can pick up after manifest-import runs? > network-routing.xml > > Comments explaining the reason for each dependency would be appreciated. accepted. > svc/seed/Makefile > > I'm not sure network/routing needs to be in the seed - I can't think of > a reason why it's needed to get any supported configuration to boot to > the point that manifest-import runs. I realize that it's because you > placed a dependency on this in network/initial, but it doesn't seem to > me that the dependency is necessary - routes come and go, nobody should > be depending on whether they're set up or not. understood. i'll excise network/routing from the seed. and thanks again for your help! i'll follow up with an updated webrev shortly, once i've tested the above changes. alan This message posted from opensolaris.org _______________________________________________ networking-discuss mailing list [email protected]
