Alok,
General comments:
-----------------
Check all files for up to date Copyright date: 2008 -> 2009
Some files have the below additional Copyright; I don't think
its needed anymore:
# Copyright (c) 1984, 1986, 1987, 1988, 1989 AT&T.
# All rights reserved.
usr/src/cmd/Makefile.cmd
------------------------
45 - nit - Why not just call this ROOTLIBSVCSHARE ?
usr/src/cmd/auto-install/config/Makefile
----------------------------------------
22 - 2008 -> 2009
usr/src/cmd/auto-install/config/get_manifest
--------------------------------------------
27 - should this be PATH=/usr/bin:${PATH} instead?
39 - Any particular reason why this dir was chosen?
Could this location be passed into get_manifest as
an argument instead?
81,83,85 - can these lines be tabbed over?
usr/src/cmd/auto-install/svc/Makefile
-------------------------------------
22 - 2008 -> 2009
usr/src/cmd/auto-install/svc/auto-installer
-------------------------------------------
75 - Shouldn't we explicitly compare string against "enable"
here before echo'ing?
usr/src/cmd/auto-install/svc/auto-installer.xml
-----------------------------------------------
22 - 2008 -> 2009
usr/src/cmd/auto-install/svc/manifest-locator
---------------------------------------------
78-82, 102-104 - This should be done in get_manifest itself.
57-60 - Can we loop and process instead of processing a set
number of args?
70 - I'd rather see this code be a bit more generic across
boot style here, but I guess that's going to have to wait
until we normalize our args for the 'net' boot case with
media boot.
76,77 - So this means that 'install' and 'prompt' are
positional then. Did we consider this when we originally
chose these paramenters?
107 - For x86, there's no equivalent for booting into
the media without starting auto-install then? If one were
to edit a grub menu entry and remove aimanifest=, it'd
seem we'd just try to do service_discovery and die?
usr/src/cmd/auto-install/svc/manifest-locator.xml
-------------------------------------------------
22 - 2008 -> 2009
Should manifest-locator also depend on some sort of
networking?
usr/src/cmd/distro_const/auto_install/ai_sparc_image.xml
--------------------------------------------------------
504 - nit - Can we drop the " for sparc" part of the message?
Otherwise, shouldn't we name the script to be sparc specific?
usr/src/cmd/distro_const/utils/grub_setup.py
--------------------------------------------
182 - Since its defined, and also because of the comment
at line 187, shouldn't this be
"else if (GRUB_SETUP_TYPE == "livecd"):"
to be specific?
Also the comment at 187 should move above 183 it seems.
usr/src/cmd/install-tools/Makefile
----------------------------------
37,39,50,57 - nit - can you tab the values over one more to be
consistent with the rest of the file?
usr/src/cmd/slim-install/svc/Makefile
-------------------------------------
22 - 2008 -> 2009
usr/src/cmd/slim-install/svc/live-root-fs-minimal.xml
-----------------------------------------------------
23 - 2008 -> 2009
87 - Is there any other way of making this determination
for x86?
usr/src/cmd/slim-install/svc/live-root-fs-minimal.xml
-----------------------------------------------------
41 - Why isn't this instance just named "default" then
if both instances of our filesystem/root depend on it?
usr/src/cmd/slim-install/svc/live-usr-fs.xml
--------------------------------------------
23 - 2008 -> 2009
usr/src/cmd/slim-install/svc/live_fs_include.sh
-----------------------------------------------
26-27 - Was this cut'n'pasted from somewhere? I don't
think we need that anymore.
usr/src/cmd/slim-install/svc/net-fs-root
----------------------------------------
26-27 - Same comment.
usr/src/lib/libtransfer/transfer_mod.py
---------------------------------------
This isn't really a "timeout" then, its a "retries" variable.
Can we name it as such throughout the code?
usr/src/pkgdefs/SUNWauto-install/prototype_com
----------------------------------------------
43 - If this is really usr/sbin/get_manifest, then you
need a usr/sbin line defined in this chunk as well.
usr/src/pkgdefs/SUNWslim-utils/prototype_com
--------------------------------------------
76 - Why is this needed if the scripts mkdir it?
thanks,
-ethan
Alok Aggarwal wrote:
> This is a code review request for the Bootable AI project [1].
>
>> From a high level, the changes proposed here are -
>
> a) Refactoring filesystem/root:live-media into the
> following components -
>
> filesystem/root-minimal:live-media (minimal bringup for root fs)
> filesystem/root:live-media (root fs bringup specific to media boot)
> filesystem/root:net (root fs bringup specific to network boot)
>
> b) Refactoring the auto-installer service into -
>
> application/manifest-locator (locate appropriate manifest to use)
> application/auto-installer (invoke the actual auto-install)
>
> c) DC changes to create a bootable sparc image
>
> d) DC changes for creating a custom grub menu
>
> e) Changes to auto-install and the transfer module to have a
> built in timeout for the initial 'pkg' contact
>
> The last one (e) shows up in the webrev but still needs more
> work and testing. I will send a follow on webrev some time next
> week that has (e) in a more finished state.
>
> The webrev location is -
>
> http://cr.opensolaris.org/~aalok/bootable.ai/
>
> I would like to solicit feedback on this wad by Thursday, 12pm PT.
> If you intend to review this code (or portions of it), please let
> me know so I can plan accordingly.
>
> Thanks,
> Alok
>
> [1]
> http://www.opensolaris.org/os/project/caiman/Bootable_AI_Image/bootable.ai.spec.txt
>
>
> _______________________________________________
> caiman-discuss mailing list
> caiman-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
