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

Reply via email to