Hi Ethan,
Thanks for the review.
On Thu, 17 Sep 2009, Ethan Quach wrote:
> General comments:
> -----------------
> Check all files for up to date Copyright date: 2008 -> 2009
Will do.
> 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.
I can't believe I gave undue credit to AT&T.
> usr/src/cmd/Makefile.cmd
> ------------------------
> 45 - nit - Why not just call this ROOTLIBSVCSHARE ?
Is there precedent to call it ROOTLIBSVCSHARE instead?
> 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?
Changed.
> 39 - Any particular reason why this dir was chosen?
No particular reason, should it be something else?
> Could this location be passed into get_manifest as
> an argument instead?
Good idea.
> 81,83,85 - can these lines be tabbed over?
Done.
> 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?
Yep, changed.
> 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.
I don't think get_manifest should make the assumption
that it can get called just on the console. In this
specific case, the caller knows that it will be called in a
certain scenario so it seems that the caller should do
the appropriate setup prior to calling get_manifest.
> 57-60 - Can we loop and process instead of processing a set
> number of args?
Sure, I can look into that.
> 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.
I do think that we need to normalize our args for the
'net' case in order to address accidental reboot in AI.
> 76,77 - So this means that 'install' and 'prompt' are
> positional then. Did we consider this when we originally
> chose these paramenters?
Yep.
> 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?
Good catch, that's a bug. I've changed it so that if it's
aimanifest=, the auto-installer service will be disabled
and manifest-locator will exit with SMF_EXIT_OK. And, drop
you into a shell.
> usr/src/cmd/auto-install/svc/manifest-locator.xml
> -------------------------------------------------
> 22 - 2008 -> 2009
>
> Should manifest-locator also depend on some sort of
> networking?
manifest-locator depends upon filesystem/minimal and
through the dependency chain ends up depending upon
system/identity:node and indirectly network/physical.
> 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?
"for sparc" dropped
> 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.
Changed.
> 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?
All the changes to this file will go away as part
of deleting getbootargs.
> 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?
I don't think so, our code uses the same check all over
the place.
> 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?
Renamed to "default".
> 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.
It's gone.
> usr/src/cmd/slim-install/svc/net-fs-root
> ----------------------------------------
> 26-27 - Same comment.
Gone.
> 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?
Agreed, changed.
> 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.
Added.
> usr/src/pkgdefs/SUNWslim-utils/prototype_com
> --------------------------------------------
> 76 - Why is this needed if the scripts mkdir it?
Removed.
Thanks,
Alok
