Thanks for reviewing this again. I took most of your
input. For the questions you had or the things I
didn't take, I have responded below.
Edward Pilatowicz wrote:
- could you propegate back your common changes to the original file?
I don't want to complicate this project with the additional
changes to the sn1 brand and the corresponding additional testing.
I filed bug:
6888642 sn1 brand cleanup
so that we can track that work as a separate integration.
- there is duplicated code here from the pkg(5) brand common.ksh. perhaps
the common code should go in /usr/lib/brand/shared/common.ksh?
get_zonepath_ds() is not common since the ipkg brand has the additional
dependency on the global zone BE which does not exist for the solaris10
- in create_active_ds(), newly created datasets will have different
values from pre-existing datasets. new datasets will inherit the
mountpoint and zoned properties while existing datasets will have
these explicitly set. (if your worried about these having incorrect
values for existing datasets, perhaps you should re-inherit them
instead of setting them.)
We don't want to inherit these, we want to set them. The values will
change as the zone gets detached/attached so we always want to set
the values we need.
- in sysunconfig_zone(), the comment says it sleeps 10 seconds, but then
it does 10 iterations of a loop with sleep 10 in it. i feel like
i've made this exact same code review comment to you recently. is
this code duplicated from the ipkg p2v code?
No, it came from the native p2v, just as the ipkg code did. I made the
fix here that I also made for the ipkg work.
- in sysunconfig_zone(), if the zone never gets to single-user mode then
should we return 1 instead of charging ahead and trying to run
sys-unconfig? (since in that case sys-unconfig could hang.)
I think its worth trying anyway since there may be something else
going on and we don't know for sure if sys-unconfig will hang.
- get_image_emul_version(), doesn't this essentially duplicate the
functionality provided by the /usr/lib/brand/solaris10/version file
delivered via s10?
in both cases we're deriving an emulation version based on the s10
bits within the zone. could you explain why this is necessary and
under what conditions each versioning mechanism will be used?
I changed this so that we still check for the minimum KU and
we now use the optional version file from S10 to determine if
we need a specific version of the emulation.
- get_image_emul_version(), does an == comparison on the KU. that means
whenever a new KU is release, we'll need to update this code. does
that mean you forsee verification test runs for each s10 KU, and a
subsequent update to this code once the tests complete? if so please
add comments to the code explaning this.
No, thats incorrect. A new KU will always incorporate the old KU.
For example, if you were running the S10u6 KU and then installed
the S10u9 KU, your system would then look like it had the u6, u7,
u8 and u9 KUs installed.
- you have the following comment:
# Try to create a zfs dataset for the zonepath (this is automatically
# done by zoneadm for the install subcommand but not for attach).
why not change zoneadm attach to be consistent with install and
create these dataset when possible. (seems like that would benifit
the ipkg brand as well.)
I filed the following bug for that enhancement but I don't
want to make that part of this project.
6888652 zoneadm attach should try to create a zfs dataset
- since the zc_name, zc_value, and zc_string are all arrays embedded
within the zfs_cmd_t, there is no reason to use uucopy to access them
Yes, since these are embedded arrays, they must be copied, we can't
simply do an assignment. I did change the uucopy to bcopy so that
I didn't have to do the cast.
- in s10_zone, in the ZONE_GETATTR() case, why isn't S10_TRUSS_POINT_5
used instead of S10_TRUSS_POINT_3?
Because the first 3 parameters are required for a truss point. That is,
S10_TRUSS_POINT_0 takes 3 parameters, S10_TRUSS_POINT_3 takes 6, which
is the number of parameters we're passing.
zones-discuss mailing list