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 cp"d 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.6666646/
>
> 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 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.)
_______________________________________________
zones-discuss mailing list
zones-discuss@opensolaris.org

Reply via email to