On Fri, 2009-08-07 at 19:09 +0200, Erik Nordmark wrote: > Some more review: > > net/if_types.h: OK > os/policy.c: OK > os/priv_defs: OK > > os/zone.c: There is a comment > 6034 * Add an data link name for the zone. Does not check for duplicates. > but the code now checks for duplicates for the same zone. > Makes sense to update the comment.
ACCEPT: Yes, the comment needs fixing. It previously didn't check for duplicates. > I assume that there is some user-space logic so that bge3 can be > assigned to the zone for both IPv4 and IPv6 (which is what the old > comments say is the need for allowing duplicates for the same zone.) Or > was that logic just unnecessary even before your changes? That comment and the justification for not checking for duplicates didn't make sense before. Assigning a link to a zone involves setting the link's zone property. When a link belongs to a zone, it can be plumbed for IPv4 or IPv6, but the concept of assigning IPv4 or IPv6 separately doesn't make sense. The link is the thing that is assigned to the zone. > You've made zone_add_datalink and zone_remove_datalink no longer be > static. But they are a bit hard to use from the kernel in general since > they assume a syscall called them (the use set_errno). Do you use them > from elsewhere in the kernel? If so it probably make sense to have them > return an errno just like zone_check_datalink. Otherwise keep them as > static functions. ACCEPT: Only zone_check_datalink() is meant to be used from the kernel. All other functions are only used from syscall context and need to be static. Will fix. > > KM_SLEEP allocation while holding zone_lock isn't a good idea at > 6225 idarray = kmem_alloc(sizeof (datalink_id_t) * idcount, > KM_SLEEP); ACCEPT: Indeed, KM_NOSLEEP is more appropriate here. > usr/src/lib/brand/native/zone/platform.xml: OK > usr/src/lib/brand/native/zone/config.xml: OK > > Post integration you need to make sure those two file changes also > appear in the ipkg brand for opensolaris. ACCEPT: Yes, I'll be in contact with Dan Price to make sure that gets done. > vplat.c: Can we remove the dladm_setzid() from the system as a result of > vplat.c no longer using it? ACCEPT: Yes, in fact, I've removed it. ;-) (see changes to libdllink.h and libdllink.c) Thanks again, -Seb _______________________________________________ networking-discuss mailing list [email protected]
