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 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_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: 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 firstname.lastname@example.org