Re: [zones-discuss] s10 brand Phase I webrev

2009-10-06 Thread Jerry Jelinek

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.


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


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


--
usr/src/lib/brand/solaris10/s10_support/s10_support.c



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


--
usr/src/lib/brand/solaris10/zone/attach.ksh

- 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


--
usr/src/lib/brand/solaris10/s10_brand/common/s10_brand.c

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


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.

Thanks again,
Jerry

___
zones-discuss mailing list
zones-discuss@opensolaris.org


Re: [zones-discuss] s10 brand Phase I webrev

2009-10-06 Thread Jerry Jelinek

Jordan,

Thanks for reviewing this again.  I took most of your
input.  For the things I didn't take, I have responded
below.

Jordan Vaughan wrote:

usr/src/uts/common/brand/solaris10/s10_brand.c

1260-1261,1286-1287,1313,etc.: Couldn't we make arg1 a zoneid_t, arg2 an
int, arg3 a char *, and arg4 a size_t and eliminate some of the casts in
s10_zone() (as well as some of the automatic variables, e.g., buf and
bufsize)?


Since thats not always the types of the parameters passed, I don't
want to change this since it would complicate any work we might
do in the future.


--
usr/src/lib/brand/solaris10/s10_support/s10_support.c

get_image_emul_version(): I agree with Ed that get_image_emul_version()
is superfluous.  Now that I've thought about it,
$ZONEROOT/usr/lib/brand/solaris10/version should be sufficient for the
brand to determine whether it can host the associated S10C.  All we need
to do is hard-code the maximum version number supported by the brand
(for example, as a preprocessor constant), fetch the version number
stored in $ZONEROOT/usr/lib/brand/solaris10/version (or zero if the file
does not exist), check whether the latter exceeds the former, and set
the brand's emulation number to that stored in
$ZONEROOT/usr/lib/brand/solaris10/version.


See my response to Ed on what I did here.  The check for the minimal
KU is not superfluous, but I did rewrite this as I explained to Ed.

Thanks again,
Jerry

___
zones-discuss mailing list
zones-discuss@opensolaris.org


Re: [zones-discuss] s10 brand Phase I webrev

2009-10-06 Thread Edward Pilatowicz
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:
---8---
e...@mcescher$ zfs get -o source mountpoint,zoned
export/zones/z1/ROOT/zbe
SOURCE
inherited from export/zones/z1/ROOT
inherited from export/zones/z1/ROOT
---8---

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?

ed
___
zones-discuss mailing list
zones-discuss@opensolaris.org


Re: [zones-discuss] s10 brand Phase I webrev

2009-10-06 Thread Peter Memishian

  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.

I was wondering about this too.  Indeed, there seems be a sizeable amount
of duplicated code now.  Why is this the right design?

--
meem
___
zones-discuss mailing list
zones-discuss@opensolaris.org


Re: [zones-discuss] s10 brand Phase I webrev

2009-10-06 Thread Jerry Jelinek

Ed,

Edward Pilatowicz wrote:

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


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


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:
---8---
e...@mcescher$ zfs get -o source mountpoint,zoned
export/zones/z1/ROOT/zbe
SOURCE
inherited from export/zones/z1/ROOT
inherited from export/zones/z1/ROOT
---8---

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?


Sure.


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


Sure.


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


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

Thanks,
Jerry

___
zones-discuss mailing list
zones-discuss@opensolaris.org


Re: [zones-discuss] s10 brand Phase I webrev

2009-10-06 Thread Jerry Jelinek

Peter Memishian wrote:

  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.

I was wondering about this too.  Indeed, there seems be a sizeable amount
of duplicated code now.  Why is this the right design?


Because the sn1 brand is an internal brand for testing
and is not delivered to customers.  Once the solaris10
brand is integrated, the sn1 brand will no longer serve
its original role and could even be removed from the source
tree if we wanted to.

Thanks,
Jerry
___
zones-discuss mailing list
zones-discuss@opensolaris.org


Re: [zones-discuss] s10 brand Phase I webrev

2009-10-06 Thread Edward Pilatowicz
On Tue, Oct 06, 2009 at 12:15:23PM -0600, Jerry Jelinek wrote:
 Ed,

 Edward Pilatowicz wrote:
 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).

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


then please make sure to update the bug with a detailed list of all the
differences between the two.  (should be easy since i already hilighted
all the differences in my code review comments.)

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

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


yes please.  otherwise some anal retentive person who is debugging a
problem will get distracted by the fact that the buf pointer and size
values are invalid.

ed
___
zones-discuss mailing list
zones-discuss@opensolaris.org


Re: [zones-discuss] s10 brand Phase I webrev

2009-10-06 Thread Jerry Jelinek

Ed,

Edward Pilatowicz wrote:

On Tue, Oct 06, 2009 at 12:15:23PM -0600, Jerry Jelinek wrote:

Ed,

Edward Pilatowicz wrote:

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

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



then please make sure to update the bug with a detailed list of all the
differences between the two.  (should be easy since i already hilighted
all the differences in my code review comments.)


Sure.  I used the file list from your comments but I'll go ahead
and paste all of them in.


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

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



yes please.  otherwise some anal retentive person who is debugging a
problem will get distracted by the fact that the buf pointer and size
values are invalid.


Will do.

Thanks,
Jerry


___
zones-discuss mailing list
zones-discuss@opensolaris.org


Re: [zones-discuss] s10 brand Phase I webrev

2009-10-06 Thread Edward Pilatowicz
On Tue, Oct 06, 2009 at 12:18:21PM -0600, Jerry Jelinek wrote:
 Peter Memishian wrote:
   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.

 I was wondering about this too.  Indeed, there seems be a sizeable amount
 of duplicated code now.  Why is this the right design?

 Because the sn1 brand is an internal brand for testing
 and is not delivered to customers.  Once the solaris10
 brand is integrated, the sn1 brand will no longer serve
 its original role and could even be removed from the source
 tree if we wanted to.


really?  i'd have to disagree.  i was actually expecting that when
nevada dies we'd have to update the sn1 brand to work on opensolaris.  i
always thought you forked the code because that was faster than
re-factoring it to be common.

the sn1 brand is still the ideal method for testing the brandz framework
itself.  using the s10 brand to test the brandz framework is ok, but
then you're also testing the s10 emulation layer, and that emulation is
far from complete.  (witness all the currently unsupported s10
functionality.)

as an example, take a look at todds current bug.  from todds (a
developers) perspective, i think it'd be much better to reproduce the
problem, debug it, fix it, and test it on the sn1 brand than on the s10
brand.  the sn1 brand makes brandz framework development and regression
testing easier for developers.  (it's actually a bug, and a dis-service
to ourselves, that we never integrated it into our automated zones
testing.)

the fact that the sn1 code is also for the most part duplicated in the
s8 and s9 brands is also lousy.  hopefully no one ever makes a
successfull business case for porting those to opensolaris.

ed
___
zones-discuss mailing list
zones-discuss@opensolaris.org


Re: [zones-discuss] s10 brand Phase I webrev

2009-10-06 Thread Jerry Jelinek

Peter Memishian wrote:

   I was wondering about this too.  Indeed, there seems be a sizeable amount
   of duplicated code now.  Why is this the right design?
  
  Because the sn1 brand is an internal brand for testing

  and is not delivered to customers.  Once the solaris10
  brand is integrated, the sn1 brand will no longer serve
  its original role and could even be removed from the source
  tree if we wanted to.

I don't see how that addresses the primary point, which is that Solaris
brands seem to suffer from code duplication.  Are you asserting that the
amount of code duplication between the sn1 and solaris10 brands is unique
to that situation and is not something that will occur again when we cons
up the next solarisX brand?


Yes.  If we were to ship another brand that was
fundamentally similar to the solaris10 brand (e.g. the solaris9
brand on Solaris Next), then I think it would make sense for
that project to try to make more of the code common with the
then currently shipping solaris10 brand.  However, I don't think
that is necessary for a brand we don't ship (but I can also
understand if you don't agree with me).  Once the solaris10
brand is integrated, I would hope that we would focus our
limited resources on the one brand we do ship across all
architectures and I would anticipate that the sn1 brand will be
of limited usefulness (since its main reason for existence is
to test brandz on sparc).  If we do anything else with sn1
after this brand integrates is outside the scope of this project
and not something we've even discussed (other than my commitment
to fix the sn1 brand per Ed's code review comments).  Of course,
its up to future brand projects to evaluate what makes the most
sense for their project and I can't commit those hypothetical
projects to anything.

Thanks,
Jerry


___
zones-discuss mailing list
zones-discuss@opensolaris.org


Re: [zones-discuss] s10 brand Phase I webrev

2009-10-06 Thread Jerry Jelinek

Ed,

Edward Pilatowicz wrote:

really?  i'd have to disagree.  i was actually expecting that when
nevada dies we'd have to update the sn1 brand to work on opensolaris.  i
always thought you forked the code because that was faster than
re-factoring it to be common.


No, that wasn't my thinking, but as I said, we've never discussed
the future of the sn1 brand.  If we think this is an important
issue, we can look at it.  From looking at the webrev, I see
maybe 11 source files (not counting makefiles, pkging files or
configuration files) that might be candidates for commonality (i.e.
files with 10% lines of difference).  This doesn't seem like a
burning issue to me.

Thanks,
Jerry


___
zones-discuss mailing list
zones-discuss@opensolaris.org


Re: [zones-discuss] s10 brand Phase I webrev

2009-10-06 Thread Peter Memishian

   I don't see how that addresses the primary point, which is that Solaris
   brands seem to suffer from code duplication.  Are you asserting that the
   amount of code duplication between the sn1 and solaris10 brands is unique
   to that situation and is not something that will occur again when we cons
   up the next solarisX brand?
  
  Yes.  If we were to ship another brand that was
  fundamentally similar to the solaris10 brand (e.g. the solaris9
  brand on Solaris Next), then I think it would make sense for
  that project to try to make more of the code common with the
  then currently shipping solaris10 brand.  However, I don't think
  that is necessary for a brand we don't ship (but I can also
  understand if you don't agree with me).

Indeed, I don't agree, for three reasons:

   1. You are establishing a precedent that the way we develop new
  Solaris brands is via cut-and-paste in a manner that's expedient for
  the implementor.  When the next brand comes along, this existing
  example will likely be used as justification for why they should not
  be burdened with the task of properly factoring the software.

   2. Shipping or not, the sn1 codebase is still part of our product
  and needs to be maintained.  By forking the source, we are 
  doubling the cost of that maintenance and making it easy for things
  to get out of sync (and indeed, the tax of maintaining these
  parallel brands has already started to be levied before the
  solaris10 brand has even integrated).  We've all seen this many
  times but yet we keep doing it.

   3. It is precisely because of our limited resources that we must
  adhere to best practices and commonize code rather than fork it.

All that said, ultimately this is an issue for you and the rest of the
zones team to resolve, so I'll bow out of this, but I strongly disagree
with the approach that's been taken.

-- 
meem
___
zones-discuss mailing list
zones-discuss@opensolaris.org


Re: [zones-discuss] s10 brand Phase I webrev

2009-10-05 Thread Jordan Vaughan

On 10/ 1/09 05:40 AM, Jerry Jelinek wrote:

Edward Pilatowicz wrote:

i'm not done yet, but i've attached what i've got so far.


Ed,

Thanks for your comments.  I'll start to work through
these while we're waiting for the rest of your input and
respond if there is anything we're not going to address.

Thank again,
Jerry
___
zones-discuss mailing list
zones-discuss@opensolaris.org


Hi Jerry,

I have a few nits and questions aside from Ed's.

Thanks,
Jordan


--
usr/src/lib/brand/solaris10/cmd/s10_automount.sh
usr/src/lib/brand/solaris10/cmd/s10_automountd.sh

Shouldn't the scripts' parameters be included at the end of the last
line (the exec command) as in s10_isaexec_wrapper.sh?

--
usr/src/lib/brand/solaris10/librtld_db/common/solaris10_librtld_db.c

The ps_plog() invocation in s10_ldb_fini32() displays lx_ldb_fini when
it should be s10_ldb_fini (right?).  According to the diff, the sn1
version also uses lx_ldb_fini.

--
usr/src/uts/common/brand/solaris10/s10_brand.c

165-171: Are we going to retain the lx brand comments?  I remember
asking this two or three months ago, but someone answered that we wanted
it to look the same as in the lx brand file from which the comment
originated.  Why?

740-743: I could've simplified this a bit by combining both cases.
These lines can be condensed to

case CT_TGET:
case CT_TSET:
return (ctfs_ioctl(rval, fdes, cmd, arg));

927-928: I could've improved this comment by stating that the path of
the *dynamic linker* is the second parameter of s10_native_exec().

1260-1261,1286-1287,1313,etc.: Couldn't we make arg1 a zoneid_t, arg2 an
int, arg3 a char *, and arg4 a size_t and eliminate some of the casts in
s10_zone() (as well as some of the automatic variables, e.g., buf and
bufsize)?

1298: Shouldn't we move this truss point below the switch block?  As it
currently stands, if a process issues SYS_zone to get an attribute of
the global zone other than ZONE_ATTR_NAME and ZONE_ATTR_BRAND, then
truss would report two SYS_zone syscalls instead of one.

--
usr/src/lib/brand/solaris10/s10_support/s10_support.c

289-296: Isn't this whole loop simply looking for SUNWcakr's pkginfo
file in the zone?  If so, then looping through the zone's /var/sadm/pkg
directory's entries for SUNWcakr is superfluous: get_ku_patchlist()
could simply construct the path
$ZONEPATH/root/var/sadm/pkg/SUNWcakr/pkginfo and stat() will fail if it
doesn't exist.  (Are we planning to examine other packages for patch lists?)

get_image_emul_version(): I agree with Ed that get_image_emul_version()
is superfluous.  Now that I've thought about it,
$ZONEROOT/usr/lib/brand/solaris10/version should be sufficient for the
brand to determine whether it can host the associated S10C.  All we need
to do is hard-code the maximum version number supported by the brand
(for example, as a preprocessor constant), fetch the version number
stored in $ZONEROOT/usr/lib/brand/solaris10/version (or zero if the file
does not exist), check whether the latter exceeds the former, and set
the brand's emulation number to that stored in
$ZONEROOT/usr/lib/brand/solaris10/version.

467,471-472,476-477: The first conditional can be changed to argc != 3
and the other two can be deleted along with their invocations of usage().

___
zones-discuss mailing list
zones-discuss@opensolaris.org


Re: [zones-discuss] s10 brand Phase I webrev

2009-10-05 Thread Jerry Jelinek

Jordan Vaughan wrote:

I have a few nits and questions aside from Ed's.


Jordan,

Thanks for looking this over.  I'll address these once
I finish going through Ed's comments.

Thanks again,
Jerry
___
zones-discuss mailing list
zones-discuss@opensolaris.org


Re: [zones-discuss] s10 brand Phase I webrev

2009-10-01 Thread Jerry Jelinek

Edward Pilatowicz wrote:

i'm not done yet, but i've attached what i've got so far.


Ed,

Thanks for your comments.  I'll start to work through
these while we're waiting for the rest of your input and
respond if there is anything we're not going to address.

Thank again,
Jerry
___
zones-discuss mailing list
zones-discuss@opensolaris.org


Re: [zones-discuss] s10 brand Phase I webrev

2009-10-01 Thread Edward Pilatowicz
i've finished looking through the rest of the files and my comments are
attached.

ed

On Tue, Sep 29, 2009 at 05:09:26PM -0600, Jerry Jelinek wrote:
 Edward Pilatowicz wrote:
 - also, since the s10 brand is derived from the sn1 brand, could you
 please ensure that all the new s10 brand that are being created are
 derived from the corresponding sn1 brand files?  ie, the s10 brand files
 which are derived from sn1 brand files should be created via hg cp ...
 instead of cp ...; hg new ... (this will preserve the sn1 brand
 revision history when looking at s10 brand files.)

 additionally, danek has an enhanced version of webrev where, if the s10
 branded files are hg cpd from the sn1 files, we'll just see the deltas
 against the sn1 files (instead of having all these files show up as new).

 I've generated a new webrev using some improvements Ed made to
 webrev.  This, combined with the use of 'hg copy', make the
 webrev much smaller and easier to review.  I've uploaded
 the new webrev to:

 http://cr.opensolaris.org/~gjelinek/webrev.646/

 Please get me any comments by the end of the week so that we
 have time to make the necessary changes and rerun the tests before
 we integrate.

 Thanks,
 Jerry
--
usr/src/lib/brand/solaris10/s10_support/s10_support.c

- s10_err() should probably write the error to stderr instead of stdout.

- s10_verify(), you don't support adding sound devices, but you do
  support other devices?  seems kinda arbitrary?  perhaps a comment
  explaining this would help?

- s10_verify(), why are zfs fsent entries experimental?  zone
  administrators don't have any abilities to manipulate properties on
  those types of zfs filesystem.  from the zones perspective they are
  treated the same as ufs, tmpfs, vxfs, etc.  only POSIX operations are
  permitted on these filesystems.  (afaik.)

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

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

--
usr/src/lib/brand/solaris10/zone/clone.ksh

- you should check for errors from mkdir and chmod

--
usr/src/lib/brand/solaris10/zone/p2v.ksh

- shouldn't safe_rm() go into /usr/lib/brand/shared/common.ksh

- should safe_rm return an error if the object exists but isn't
  a file?

- please check error codes from mktemp

- please check error codes from chown and chmod.

--
usr/src/lib/brand/solaris10/zone/image_install.ksh

- trap_cleanup(), comment talks about IPDs

- why set EXIT_CODE=$ZONE_SUBPROC_OK at 249?  if we ctrl-C the
  process after this and the zone won't have the install log in
  it but it will still be listed as installed.

- can't you use safe_dir() to verify /var in the zone?

--
usr/src/lib/brand/solaris10/zone/attach.ksh

- trap_cleanup(), comment talks about IPDs

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

- same comments about zfs property inheritance vs explicit setting
  that i made about create_active_ds() in common.ksh apply here.

  actually, why can't you just add an argument to create_active_ds() to
  indicate if the datasets must already exist, and then just invoke
  that here?

- the two /var/log  comments i made about image_install.ksh apply here.
  so why not create a function that does all the /var/log checks and
  copies the log files into the zone and put it in common.ksh?

--
usr/src/lib/brand/solaris10/s10_brand/common/s10_brand.c

- the d, s, and val parameters should have parens around them in
  the zfs_assign macro.

- there is nothing zfs specified about the zfs_assign macro.
  this would be more appropritely names struct_member_assign
  or struct_member_copy.

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

- given the level of indentation in s10_auditsys(), wouldn't it make
  sense to reduce the indentation by doing:
if (bsmcmd != BSM_AUDITCTL)
return (__systemcall(rval, SYS_auditsys + 1024,
bsmcmd, a0, a1, a2));

- the comment above s10_exec_native() says:

Re: [zones-discuss] s10 brand Phase I webrev

2009-09-30 Thread Edward Pilatowicz
i'm not done yet, but i've attached what i've got so far.
ed

On Tue, Sep 29, 2009 at 05:09:26PM -0600, Jerry Jelinek wrote:
 Edward Pilatowicz wrote:
 - also, since the s10 brand is derived from the sn1 brand, could you
 please ensure that all the new s10 brand that are being created are
 derived from the corresponding sn1 brand files?  ie, the s10 brand files
 which are derived from sn1 brand files should be created via hg cp ...
 instead of cp ...; hg new ... (this will preserve the sn1 brand
 revision history when looking at s10 brand files.)

 additionally, danek has an enhanced version of webrev where, if the s10
 branded files are hg cpd from the sn1 files, we'll just see the deltas
 against the sn1 files (instead of having all these files show up as new).

 I've generated a new webrev using some improvements Ed made to
 webrev.  This, combined with the use of 'hg copy', make the
 webrev much smaller and easier to review.  I've uploaded
 the new webrev to:

 http://cr.opensolaris.org/~gjelinek/webrev.646/

 Please get me any comments by the end of the week so that we
 have time to make the necessary changes and rerun the tests before
 we integrate.

 Thanks,
 Jerry
--
usr/src/lib/brand/solaris10/s10_brand/Makefile

- could you propegate back your common changes to the original file?

--
usr/src/lib/brand/solaris10/s10_support/Makefile

- instead of:
../../../../uts
  could you do:
$(SRC)/uts

- could you propegate back your common changes to the original file?

--
usr/src/lib/brand/solaris10/librtld_db/Makefile.com

- could you propegate back your common changes to the original file?

--
usr/src/lib/brand/solaris10/zone/config.xml

- could you propegate back your common changes to the original file?

--
usr/src/lib/brand/solaris10/zone/platform.xml

- could you propegate back your common changes to the original file?

--
usr/src/uts/common/brand/solaris10/s10_brand.c

- in s10_getattr() and s10_setattr(), how about just doing:
if (bufsize != sizeof (int))
  and then you don't need to bother setting bufsize in s10_getattr().

- in s10_elfexec() it seems like the changes required to make ld.so.1
  run (ie, sedp-sed_entry = NULL, up-u_auxv[i].a_un.a_val =)
  run should be backported to the sn1 brand.

--
usr/src/lib/brand/solaris10/zone/uninstall.ksh

- this seems to largely be a duplicate of the unistall script from the
  pkg(5) gate.  i only see two differences between the scripts:

- the method used to determine the zfs filesystems for the zone
  (beadm vs zoneadm) and filtering based on uuids.

- the code at the very end under the Check if there are any
  other datasets left. comment.  this one seems like a
  potential mismerge, since i would think that you'd actually
  want to keep this bit of code.

  to avoid duplicating this entire script, could you please move most
  this code into usr/lib/brand/shared and either:

- make it a common script that is called by a brand specific
  script that passes it a list of datasets to delete
- or have the script take a command line option which controls
  how the list of datasets to delete is determined.

  then after you putback the ipkg brand can be updated to use this same
  code.

--
usr/src/lib/brand/solaris10/zone/query.ksh

- also a dup of the query in the pkg(5) gate, should be common.

--
usr/src/lib/brand/solaris10/zone/s10_boot.ksh

- is_zone_dir() looks like a dup of
  /usr/lib/brand/shared/common.ksh`safe_dir()

- shouldn't safe_replace() go into /usr/lib/brand/shared/common.ksh?

--
usr/src/lib/brand/solaris10/zone/detach.ksh

- do you use anything from?
/usr/lib/brand/shared/common.ksh

- why?
ZONEROOT=$ZONEPATH/root
logdir=$ZONEROOT/var/log

--
usr/src/lib/brand/solaris10/zone/common.ksh

- shouldn't the EXIT_* definitions go in /usr/lib/brand/shared/common.ksh?

- hm.  i find the PROP_ACTIVE and libbe references strange.

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

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

- in create_active_ds(), you should check the return value for mkdir.

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

- in 

Re: [zones-discuss] s10 brand Phase I webrev

2009-09-29 Thread Jerry Jelinek

Edward Pilatowicz wrote:

- also, since the s10 brand is derived from the sn1 brand, could you
please ensure that all the new s10 brand that are being created are
derived from the corresponding sn1 brand files?  ie, the s10 brand files
which are derived from sn1 brand files should be created via hg cp ...
instead of cp ...; hg new ... (this will preserve the sn1 brand
revision history when looking at s10 brand files.)

additionally, danek has an enhanced version of webrev where, if the s10
branded files are hg cpd from the sn1 files, we'll just see the deltas
against the sn1 files (instead of having all these files show up as new).


I've generated a new webrev using some improvements Ed made to
webrev.  This, combined with the use of 'hg copy', make the
webrev much smaller and easier to review.  I've uploaded
the new webrev to:

http://cr.opensolaris.org/~gjelinek/webrev.646/

Please get me any comments by the end of the week so that we
have time to make the necessary changes and rerun the tests before
we integrate.

Thanks,
Jerry
___
zones-discuss mailing list
zones-discuss@opensolaris.org


Re: [zones-discuss] s10 brand Phase I webrev

2009-09-28 Thread Jerry Jelinek

Edward Pilatowicz wrote:

hey jerry,

do you have an updated ws+webrev where the s10 files were created using
hg cp?  (i'm waiting for that before doing a review.)

also, when were you planning to integrate?  (so i can avoid a last
minute rush.)


Ed,

I wasn't aware that this was holding you up.  I haven't
done anything on it yet.  I'll work on regenerating the
webrevs by tomorrow and send out an announcement.  We
are targeting b127 so it would be good to get any comments
this week so that there is time to respond and test any
changes.

Thanks,
Jerry
___
zones-discuss mailing list
zones-discuss@opensolaris.org


Re: [zones-discuss] s10 brand Phase I webrev

2009-09-28 Thread Edward Pilatowicz
On Mon, Sep 28, 2009 at 03:06:00PM -0600, Jerry Jelinek wrote:
 Edward Pilatowicz wrote:
 hey jerry,

 do you have an updated ws+webrev where the s10 files were created using
 hg cp?  (i'm waiting for that before doing a review.)

 also, when were you planning to integrate?  (so i can avoid a last
 minute rush.)

 Ed,

 I wasn't aware that this was holding you up.  I haven't
 done anything on it yet.  I'll work on regenerating the
 webrevs by tomorrow and send out an announcement.  We
 are targeting b127 so it would be good to get any comments
 this week so that there is time to respond and test any
 changes.


ah.  yeah.  i was waiting because i wanted to use the hg cp' workspace
to generate a webrev using daneks copy of webrev.  (since that would
show just the delta to all the copied files, there by greatly reducing
the amount of code to review.)

ed
___
zones-discuss mailing list
zones-discuss@opensolaris.org


Re: [zones-discuss] s10 brand Phase I webrev

2009-09-16 Thread Edward Pilatowicz
two high level comments to start with.

- since the s10 brand is derived from the sn1 brand, are there any
framework changes which were made to the s10 brand that should be
backported to the sn1 brand?

- also, since the s10 brand is derived from the sn1 brand, could you
please ensure that all the new s10 brand that are being created are
derived from the corresponding sn1 brand files?  ie, the s10 brand files
which are derived from sn1 brand files should be created via hg cp ...
instead of cp ...; hg new ... (this will preserve the sn1 brand
revision history when looking at s10 brand files.)

additionally, danek has an enhanced version of webrev where, if the s10
branded files are hg cpd from the sn1 files, we'll just see the deltas
against the sn1 files (instead of having all these files show up as new).

ed

On Tue, Sep 15, 2009 at 08:48:19AM -0600, Jerry Jelinek wrote:
 We've completed the development for the Phase I
 work on the solaris10 brand.  I've posted a
 full webrev at:

 http://cr.opensolaris.org/~gjelinek/webrev.646/

 Let me know if there are any comments.

 Thanks,
 Jerry
 ___
 zones-discuss mailing list
 zones-discuss@opensolaris.org
___
zones-discuss mailing list
zones-discuss@opensolaris.org


Re: [zones-discuss] s10 brand Phase I webrev

2009-09-15 Thread Peter Memishian

  We've completed the development for the Phase I
  work on the solaris10 brand.  I've posted a
  full webrev at:
  
  http://cr.opensolaris.org/~gjelinek/webrev.646/
  
  Let me know if there are any comments.

I see that ip-type=exclusive is regarded as experimental in
s10_support.c; is that planned for Phase II?

-- 
meem
___
zones-discuss mailing list
zones-discuss@opensolaris.org


Re: [zones-discuss] s10 brand Phase I webrev

2009-09-15 Thread Jerry Jelinek

Peter Memishian wrote:

  We've completed the development for the Phase I
  work on the solaris10 brand.  I've posted a
  full webrev at:
  
  http://cr.opensolaris.org/~gjelinek/webrev.646/
  
  Let me know if there are any comments.


I see that ip-type=exclusive is regarded as experimental in
s10_support.c; is that planned for Phase II?


Yes, that's right.  We haven't done any of the testing
or evaluation yet on what it will take to make exclusive
fully work for the brand but we will do that for Phase II.

Thanks,
Jerry
___
zones-discuss mailing list
zones-discuss@opensolaris.org