William Schumann wrote: > [Due to various changes, this code review has been regenerated. > Among the modifications are fixes, some new ICTs. > - IPS Python module pkg.cfgfiles is now used for adding and modifying > users. > - The install-finish script now reads options from the command line. > - 'beadm list -dH' is used to find the root dataset. > William] > > Webrev: > http://cr.opensolaris.org/~wmsch/bug-1040/ > Defect: > http://defect.opensolaris.org/bz/show_bug.cgi?id=1040 >
Meta-comment: do we need to adjust the progress percentages a bit based on moving things out of transfer_mod.py? I don't get the reason for the changes to the gui-install Makefiles. It doesn't call into ICT directly, nor does it link directly against libict. Specific comments: libict/Makefile, 61: Why build a static version? ict.c: 188: I'd do this before the check at 183 so you don't generate an error when you'd otherwise be doing nothing 200: Seems like we're completely depending on the install environment's umask here for directory permissions; wouldn't a mode of 755 be a better starting point? 322: Shouldn't there be a chmod in here too? 602: I think we should just be snapshotting the initial BE rather than the entire pool. I suppose that's what my original hacky code did, but it was wrong. ict.py: 577,623: no failsafe in OpenSolaris at this time, I'd just remove 841: dump already on a dataset, can remove 997: I fixed this as part of the changeset with bug 681 and friends, pick it up from there 1368: check whether DC phase 0 has fixed this. ls_main.c, 486: A warning of an invalid debug level seems like it would be nice. orchestrator_private.h: 134,136: do we still need these definitions? perform_slim_install.c: 1158 et al: the cast shouldn't be needed 1235: this comment is a bit out of date 1238: this condition really isn't necessary, there's no other context possible at this point, is there? Also, you didn't create this problem but the remainder of this function really has nothing to do with calling the transfer module and should be moved to a separate function called from om_perform_install(), though I suppose that can be a separate bug. 1333: is this block still necessary now that you're using be_create_snapshot()? If so, please file a bug to have it not include swap and dump in the snapshotted datasets. See note earlier about the scope of the initial snapshot. install-finish: 55: this can't happen, block can be deleted Dave
