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