crossbow discuss - Nov 2006 - [zones-discuss] [Fwd: Code review for IP Instances]

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.

Jerry

---

usr/src/cmd/zoneadm/Makefile
         ok

usr/src/cmd/zoneadm/dlprims.c

64 Why isn''t this static?  I don''t see it called in zoneadm.c.

65 & 103 Why the old style declaration?  Also, this is not correct cstyle.
     For K&R declarations the parameters should be tabbed over one tab stop.

102 Why isn''t this static?  I don''t see it called in
zoneadm.c.

131 It looks strange that now you switched styles for the function
     declaration.

168 Should this be declared as a boolean_t?

181 Why isn''t this static?  I don''t see it called in
zoneadm.c.

201 Can you improve this comment so it explains the difference between
     a level 1 and level 2 device?

204 Why isn''t this static?  I don''t see it called in
zoneadm.c.

238-240 This comment seems to contradict the comment on device_path at
     lines 199-201.

253, 256, 263 & 267 The netfd is not closed before returning.

usr/src/cmd/zoneadm/zoneadm.c
51 Why was this include added to the file?

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.

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''.

474 Missing curly braces around else clause.

489 Memory leak, need to free memory allocated at 442.

554 Is there is a missing space in the addition to the format string?

635 Why not just check for ''zid == GLOBAL_ZONEID''?

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''.

2421, 2427 & 2430 Should return B_FALSE or B_TRUE.

2968 We try not to use contractions in messages since they don''t
     internationalize easily.

2984-2992 Why don''t you just use target_zone here?

3008 Use snprintf

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.

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.

2453 This is not a very user-friendly error msg.  Can you improve it
     with something like "skipping network interface %s, used by global
zone"
     Likewise, in the rest of the changes in this code, replace
"ifname" with
     better wording such as "network interface".

2473 Comment should say "providers".

2478 I don''t think most people know what this means or what they should
     do if they see this error msg.

2530 Can you make this error msg consistent with the rest of the code.

2573 Shouldn''t this be ''inaccessible''.

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?

4290 Why was this line changed?

usr/src/cmd/zonecfg/zonecfg.c
275 Missing ip-type in this array

812 This sounds awkward.  How about "..., otherwise it must not ..."

968, 2662, 3025 Why was this line changed?

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.

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.

usr/src/cmd/zonecfg/zonecfg.h
         ok

usr/src/cmd/zonecfg/zonecfg_grammar.y
         ok

usr/src/cmd/zonecfg/zonecfg_lex.l
         ok

usr/src/cmd/zonename/zonename.c
         ok

usr/src/head/libzonecfg.h
         ok

usr/src/head/zone.h
         ok

usr/src/lib/brand/native/zone/platform.xml
         ok

usr/src/lib/brand/sn1/zone/platform.xml
         ok

usr/src/lib/libbrand/common/libbrand.c
         ok

usr/src/lib/libbrand/common/libbrand.h
         ok

usr/src/lib/libbrand/dtd/zone_platform.dtd.1
48 The last sentence in this comment looks like it is missing
''which'' after
     ''used''.

usr/src/lib/libc/port/mapfile-vers
         ok

usr/src/lib/libc/port/sys/zone.c
         ok

usr/src/lib/libzonecfg/common/libzonecfg.c
1439 Why isn''t this function leveraging the existing getrootattr()
function?

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).

usr/src/lib/libzonecfg/common/mapfile-vers
         ok

usr/src/lib/libzonecfg/dtd/zonecfg.dtd.1
         ok

usr/src/uts/common/os/zone.c
235-241 These includes are duplicated and the label.h is redundant from line
     233.

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?

     It looks like this change will have a big impact on patching since
     all of the kernel modules calling zone_key_create() will have to be
     patched at the same time as the zones kernel patch.  Is there no
     other way to make this work?

     It also looks like this change is not implemented properly in the webrev.
     I tried searching the full set of diffs looking for the modified calls to
     zone_key_create() that are in the different subsystems and I
couldn''t
     find the changes I was expecting to see (e.g.
     uts/common/gssapi/gssd_handle.c, uts/common/fs/nfs/nfs4_client.c,
     uts/common/rpc/svc.c were the files I was looking for in the webrev to
     check these changes, but there are a lot more calls to  zone_key_create()).

5192 This function can be eliminated if you just removed the test in
     zone_get_iflist() at line 5233.

usr/src/uts/common/sys/syscall.h
         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''?

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.

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