Hi Jack, Jack Schwartz wrote: > Hi Sue and Keith. > > Nice doc. Looks quite polished, even from the get-go...
Thanks! > Here are my comments: > > 2.4.4: > > I suggest a different name than validate_partition_info, since the > method can change the info, more than just validate it. Maybe > validate_and_align_partition_info()? I frown upon really long function names. What do people think about just "resize_partitions" or "align_partitions"? > > 2.4.4.4: > > For completeness, please add a reference to the doc which defines > om_validate_and_resize_disk_partitions(), since that function is > referenced here. Please do the same for other functions in the > existing installer as well. While that would be useful, the source code is readily available, and between references to functions from liborchestrator, libtd, libti, and libtransfer - plus all the ICTs - I don't think we'll have time to add all that information. In cases where a liborchestrator function is referenced (but won't be used), I believe we've given a short blurb about what the function does. > > Last sentence: rather than refer to the "new library", I would say the > "new library that replaces liborchestrator". Section 2.4.1 indicates that the new library contains functions that replace some of the functionality of liborchestrator. The new library is not a full replacement; nor is it limited to only functionality that liborchestrator had, so vocalizing the new library simply as a replacement is misleading, I think. > > 2.4.5: > > Similar comment to 2.4.4. The name doesn't reflect that the function > can change its data. Similar question: Would "align_slices" or "resize_slices" be acceptable? > > 2.5.2.3: > > Is there a way to determine the minimum amount of required disk space > dynamically? This would remove the need to update the .image_info > file whenever the system contents change. Furthermore, the size > should be easy to determine since you know how big the running system > is, and the target will be a cpio of it. Actually, using the .image_info file is simpler and more accurate. If I understand correctly, the .image_info file is generated by DC (so it's not "hand-written"). Since this is a CPIO transfer, the .image_info data should be more accurate - as opposed to trying to guess the final size, since the running system has some files compressed, and trying to guess the compression ratio could be difficult. > > 2.5.3.3: > > Similar comment to 2.5.2.3. Perhaps the recommended size can be > determined by plugging the size of the running system into a formula. Similar response. > > 2.5.5.4: > > I would at least add an assertion that all data is present, to aid in > debugging. The details section indicates that all data must be present, but doesn't indicate what is or isn't required, nor what happens if data is missing. I'll add an explicit reference to ValueError being thrown in this case. > > 2.5.6.3: > > Nit: I would leave out the last sentence. This spec is not for the > current GUI. I included that bit of info as justification for following a similar pattern. I have clarified that point. > > > 2.5.9.4: > > Just curious: how will the percentages passed to > update_install_status() be determined? The GUI breaks installation into 3 stages: TI, transfer, and ICT. It has TI hardcoded to take 15% overall, transfer takes about 84%, and ICT only 1%. During the TI stage, 5 calls into libti are made. After each call, the TI stage is assumed to be 20/40/60/80/100% complete, and the displayed percentage is adjusted accordingly (e.g. after the first call, the overall percentage is set to 20% * 15% = 3%). During the transfer stage, a more intelligent callback is registered, which indicates the completion percentage for that stage. The displayed percentage is modified based on that information - e.g., if transfer is 40% complete, then the displayed percentage is 15% (TI complete) + 40% * 84% = 49% (approx) Since ICT is assumed to be only 1%, the display is only updated just before and just after ICTs complete. For now, our plan for the Text Installer was to follow a similar pattern (perhaps adjusting the percentages based on the size of the image). I'll update the design doc with that information. > > 2.7.1: > > To be clear, you could change: > 8 slices on SPARC, 16 slices on x86 > to > 8 slices on a SPARC disk, 16 slices on an x86 Solaris2 partition Your wording is better, thanks! Side note: I need to clarify this with Frank as to how many partitions will be displayed for x86. I think the theoretical max is 16, but 10 is the convention. > > 2.8.1: > > The menu will have four items, including invocation of the DDU, and a > selection to reboot. Added. > > 2.8.3: > > I thought we'd decided that the text-mode environment would come up > with NWAM enabled by default. That way the DDU can use the network if > it has DHCP enabled. Without any network configuration built into the > DDU, at least NWAM should be enabled. That's correct. That was all decided *after* the design doc was posted, however. It will be updated. > > While on the subject of network screens, can the network screens be > among the first config screens in the installer? That way, one can > start the installer to configure the network, exit back to the menu > and run the DDU to take advantage of the newly-configured network. I don't think it's beneficial to have such a "hack" for enabling networking. A console based tool for configuring NICs is part of networking, not install. The text installer will not actually be configuring the booted environment - it will only set the files needed so that the installed system has the desired network config on first boot. However, as mentioned, NWAM will be enabled on the live media, and a shell is available for using ifconfig to set up the network. > > I would suggest a very small (1 or 2), but non-zero grub timeout. > This way one can interrupt grub to change kernel parameters for > debugging purposes if necessary. IMO, a small timeout won't be a > nuisance and could be useful. That's a good point. We can set the "hiddenmenu" GRUB option, so users don't see the GRUB menu list, but can access it if they need to debug. In that case, perhaps a timeout of 5 seconds or so would be appropriate. > > 3.1: > > Nit: in order to be consistent with the second paragraph, I would change > "linked against libncurses" > to > "linked against the nCurses library (libncurses)" Changed. Thanks for taking the time to review! - Keith > > Thanks, > Jack > > On 09/04/09 16:12, Sue Sohn wrote: >> We have posted the design document for the Text Installer at: >> >> http://www.opensolaris.org/os/project/caiman/TextInstallerProject/design_doc_v1.4.pdf >> >> >> >> Please review and provide comments by COB Tuesday, September 15th. >> >> Thanks, >> Sue >> _______________________________________________ >> caiman-discuss mailing list >> caiman-discuss at opensolaris.org >> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >
