Jerry Jelinek wrote:
Erik Nordmark wrote:
The IP Instances project is now soliciting code review comments.
I reviewed the zones portions of the webrev and my comments are
below.
Great. Thanks for your careful review.
Unless otherwise noted we've applied your suggested changes.
Responses and commentary below.
usr/src/cmd/zoneadm/dlprims.c
We were hoping the libdlpi putback would happen before us, in which case
we wouldn't need to putback this file. Given that libdlpi will integrate
after us, we will cleanup this file based on your comments. The libdlpi
team knows they should rip out this file when they integrate.
(FWIW The file is a copy from an other part of the source base.)
usr/src/cmd/zoneadm/zoneadm.c
51 Why was this include added to the file?
We've removed it.
264 The term "interface list" is not very inconsistent with the rest of the
msgs zonedm. Maybe "network interface list" would be better since
otherwise this msg is not very clear about what interfaces it is
referring to.
We will use "network interface" throughout. But be aware that the zones
book uses the term "zones interfaces" when talking about zones network
interfaces, so it probably makes sense raising a doc CR on that.
432, 468, 481 Using the term "link" is inconsistent with the rest of the
terminology in zones. Instead of "links" maybe you can say 'network
interfaces'.
Agreed.
474 Missing curly braces around else clause.
489 Memory leak, need to free memory allocated at 442.
PSARC gave us a TCR to not introduce zoneadm list -l, and instead add
zone information to
dladm show-prop
so this part of the code is gone.
554 Is there is a missing space in the addition to the format string?
Yes.
635 Why not just check for 'zid == GLOBAL_ZONEID'?
Good suggestion.
1754 & 1799 This error msg doesn't sound right. How about following the
convention from line 1748 and saying they are mutually exclusive or at
least say 'can not be used'.
1822 This error also sounds wrong. Should say "-l can only be applied to a
running zone'.
Code removed with the removal of zoneadm list -l.
2421, 2427 & 2430 Should return B_FALSE or B_TRUE.
OK. This function is now in libdladm.
2984-2992 Why don't you just use target_zone here?
Will do.
usr/src/cmd/zoneadmd/Makefile
46 Does SUNWzoneu depend on SUNWcslr now? Was SUNWzoneu pkg dependency
modified? I did not see any changes to the SUNWzoneu pkg dependencies
in the full webrev.
I have a question here, SUNWzoneu depends on SUNWcsl not SUNWcslr,
but zoneadmd is using libuuid, and libuuid is in SUNWcslr, but it's also
in SUNWcsl's pkgmap, as:
1 s none usr/lib/libuuid.so=../../lib/libuuid.so.1
Shouldn't this cause a problem if SUNWcslr is not installed?
usr/src/cmd/zoneadmd/vplat.c
2421 This is a duplicate function from zoneadm.c line 2409-2432. Can you
create a single, common function in libzonecfg.
Consider it done.
2478 I don't think most people know what this means or what they should
do if they see this error msg.
We've change it to
2457 zerror(zlogp, B_FALSE, "WARNING: skipping
unsupported network "
2458 "interface %s", nwiftabptr->zone_nwif_physical);
since DLPI style 2 providers are not supported.
4091-4116 Why are you doing this work when we are just mounting the zone?
I think you should skip adding either type if network interface in this
case. Likewise for the changes starting at 4264. Otherwise, can you
explain why you need to do this when mounting a zone?
Good point. This was a mismerge some time in the past.
4290 Why was this line changed?
Change undone. [As to "why", the indentation of the code has changed
back and forth while merging with all the Nevada changes to the file.]
usr/src/cmd/zonecfg/zonecfg.c
275 Missing ip-type in this array
Added.
812 This sounds awkward. How about "..., otherwise it must not ..."
OK
968, 2662, 3025 Why was this line changed?
L968: Because earlier we had additional properties, that were removed in
response to the discussion at PSARC inception review.
The changes are undone.
3892 This error msg is incorrect. Since zonecfg_get_iptype() returns
ZS_SHARED when ip-type is not specified, something else is broken if
zonecfg_get_iptype() returns other than Z_OK.
Yes. The new error message is
3892 zerr("%s %s", gettext("can not get"),
pt_to_str(PT_IPTYPE));
4306 Since the address property is now optional this error msg will not
print
right if the in_progress_nwiftab.zone_nwif_address has not been
filled in.
The output will be something like
a net resource with physical bge0 and address '' already exists.
I think it's acceptable.
Or do you see a need to have separate error messages for the two cases?
usr/src/lib/libzonecfg/common/libzonecfg.c
1439 Why isn't this function leveraging the existing getrootattr()
function?
We've done that [I don't think that function existed when the code was
first written.]
2064 The changes to this function break the lookup when you are only
passing
in an address as the qualifier for a lookup. This will cause zonecfg
to break when you are selecting a network interface with only the
address
as the property name (see the fill_in_nwiftab() function in zonecfg.c).
Good catch.
We also found that using the zones test suite. We'll restore the
code to be exactly as in Nevada.
usr/src/uts/common/os/zone.c
508 Why are you adding a flags parameter to the zsd create function?
I guess I don't understand why the flags have to be passed as a
parameter. Since the zsd create function gets the zone_id, can't it
just lookup the zone_flags if it needs them?
For historical reasons. But with the more recent introduction of
zone_find_by_id_nolock() in our bits we don't need the extra argument so
we'll remove it.
5192 This function can be eliminated if you just removed the test in
zone_get_iflist() at line 5233.
OK
usr/src/uts/common/sys/zone.h
268-271 Why were these moved from their former position after line 282?
Shouldn't these still be inside the '#ifdef _KERNEL'?
The ZF_* are returned as part of the new ZONE_ATTR_FLAGS (so that
user-space can see whether ZF_NET_EXCL is set or not). Thus at least
that flag needs to be visible outside the kernel. It made some sense to
make all of them visible outside the kernel.
440 & 457 See my comments for zone.c. I don't think this change was
implemented properly since the calls to zone_key_create that I checked
were not in the webrev. Also, the declaration on 440 no longer matches
the declaration on 457.
FWIW We'll also fix the missing type for the 2nd argument to
zone_key_create to be (zoneid_t) instead of (), so that the compiler
will catch any future inconsistencies.
Erik
_______________________________________________
zones-discuss mailing list
zones-discuss@opensolaris.org
