i've finished looking through the rest of the files and my comments are


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

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


- you should check for errors from mkdir and chmod


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


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


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


- 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

- 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:
        This will make pgrep and other commands that examine process'
        executable names and command-line parameters work properly.

  that really isn't correct.

  the purpose of s10_exec_native() is to execute a native process.  once
  we've done that s10_npreload.so invokes the B_S10_NATIVE brand operation
  which actually patches up the processes exec info so that pgrep and
  friends work as expected.

- in s10_zone, in the ZONE_GETATTR() case, why isn't S10_TRUSS_POINT_5
  used instead of S10_TRUSS_POINT_3?

- s10_init1m_handler(), why?

zones-discuss mailing list

Reply via email to