Hi Chris & Andrew,
Here are my comments:
--------------
usr/src/cmd/distro_const/auto_install/ai_sparc_image.xml
usr/src/cmd/distro_const/auto_install/ai_x86_image.xml
Like Jan, I am concerned that changes for supporting network
based text-installer is made directly to the AI image manifests.
When reviewing the design doc for this project, I am under the
impression that a new manifest will
be defined for this. I saw Keith's reply to this. If this is
just going to be a temporary solution, I supposed it is OK.
-----------
usr/src/cmd/distro_const/auto_install/ai_sparc_image.xml:
lines 117-121: Why is this finalizer script needed for this image, and why
is it added here? If it is truly needed
for this image, it should have been added BEFORE the "ba-init" step for it
to actually be useful.
-----------
usr/src/cmd/distro_const/auto_install/ai_x86_image.xml:
lines 112-117: again, why is this finalizer script added, and why here?
If it is really needed, should be added before the "ba-init" step.
----------
usr/src/cmd/slim-install/svc/net-fs-root
lines 152: please expand on this comment to talk about exactly what we
are looking for in "bootargs". For people who are not familiar with the
code, they might not know what is being checked.
lines 154-169: This feels like a very complicated way to check
whether the "install" option is specified. I think it will be
easier to assign the result of the prtconf command to a variable, and
then, check to see whether the " install " substring exists.
lines 184-187: I believe you can simplify this to:
if [ ${INSTALL_FLAG} == "true" ] ; then
TEXT_FLAG = "false"
fi
lines 330-363: All these commands are executed without checking for
return values. Please check return values of all the commands, and
error out as appropriate.
line 334-339: here, you are assuming that the 32bit kernel is always booted,
which might or might not be the case, please take care of cases where 64 bit
kernel is booted.
line 363: comment not needed here.
-------------
usr/src/lib/libict_pymod/ict.py:
lines 1612-1617: why is this check necessary?
---------
usr/src/lib/liborchestrator/perform_slim_install.c:
line 2364: you have %d for "source" variable, which is a string
----------
Thanks,
--Karen
On 08/31/10 12:35, Chris Navrides wrote:
Hi all,
Could we please get a code review of the modifications made to
enable 6971585 Network Text Install
This will enable a client over the network to either do an Automated
Install or a Text Install. The default in the grub menu will lead to
the text install menu. For sparc if no "-install" flag is set then it
will also default to the text-install menu.
Webrev:
http://cr.opensolaris.org/~chris_n/netboot_text_instal
<http://cr.opensolaris.org/%7Echris_n/netboot_text_install/>l/
<http://cr.opensolaris.org/%7Echris_n/netboot_text_install/>
We have done testing with both x86 and sparc using the test bed on the
10.10.45 subnet. There are no changes done to installadm on the server
side.
Thanks,
Chris & Andrew
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
