On Tue, Oct 06, 2009 at 09:47:40AM -0600, Jerry Jelinek wrote:
> Ed,
> 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.

sigh.  are you commiting to this work?  the sn1 brand always get's
neglected (says the guy who spent too much time fixing it up to be a
real brand).

also, i would have though you'd commited to doing this work when you
decided to fork the sn1 brand code instead of making it common.
besides, aside from the elfexec changes all the changes are Makefile
related, right?  that's not a whole lot of extra testing.

>> - 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?
>>      fail_fatal()
>>      get_zonepath_ds()
>>      get_active_ds()
> 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
> brand.

ok.  but if get_zonepath_ds() is not common, then why are you adding it
to usr/src/lib/brand/native/zone/common.ksh?

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

i dont' understand, currently we don't always set these values.

if we do a fresh install, "mountpoint" and "zoned" are inherited:
e...@mcescher$ zfs get -o source mountpoint,zoned
inherited from export/zones/z1/ROOT
inherited from export/zones/z1/ROOT

so why the difference for attached zones?  for attached zones you
"zfs set" the values above.  why not just "zfs inherit" instead.
(you already explicitly set them for the ROOT dataset, so they
would be correct.)

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

could you add comments explaining this?

>> ----------
>> usr/src/lib/brand/solaris10/s10_support/s10_support.c
>> - 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.

please add comments explaining this.

>> ----------
>> usr/src/lib/brand/solaris10/s10_brand/common/s10_brand.c
>> - 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.

but i thought the caller passed in 4 parameters. (in addition to the
cmd.)  why are you not printing out "buf" and "bufsize"?

zones-discuss mailing list

Reply via email to