Edward Pilatowicz wrote:
On Tue, Oct 06, 2009 at 09:47:40AM -0600, Jerry Jelinek wrote:

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

Sure.  I just don't want these coupled together.

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

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

Sorry, I meant get_active_ds(), not get_zonepath_ds().

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

OK, I misunderstood what you meant.  I'll change this.

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


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



- 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"?

Those parameters aren't that useful for debugging.  I can add them
if you'd like.


zones-discuss mailing list

Reply via email to