Hi Sue.
Here are my comments:
installadm/Makefile: OK
create-client.sh:
251: IMAGE_BOOT is used only for the X86 case, so I would move this line to
near 316 where it is used, or else combine it with 316
253-259: Seems confusing that -f specified on SPARC
without $BOOTFILE is acceptable but with $BOOTFILE is not. Based on
255, looks
like -f should always be invalid for SPARC, whether or not BOOTFILE is
specified.
I would prefer to see a setup_x86 file (similar to setup-sparc) and move
lines
289 - 415 and other SPARC-centric stuff there. Equal treatment for
SPARC and
x86 = better modularity.
install-common.sh:
~159: Please add a comment that create_menu_lst_file is for x86.
178: Seems that the only time when $Menufile is re-created is when the
kernel$
line is missing. What if it is corrupted in some other way? It would
be more
robust just to re-creating $Menufile each time.
Being new to this code, I'll ask: why is create_menu_lst_file called
from both
create_client and setup-tftp-links?
installadm.c:
407, 465: hardwired constant. Instead of 256, you can include
<limits.h> and
use _POSIX_HOST_NAME_MAX . I checked and getconf command uses this to
return the
host name length. (If you look hard enough, you'll see that different
parts of
the OS use two different numbers: 64 and 256... That said, 256 is the
safest.
467: hardwired constant: The only place it is used is to store output from
inet_ntoa(), which would be a max of 40 chars (8 4-hex-digit values with
: in
between, plus terminating NULL)
installadm.h:
59,60: I would prefer more parallelism between the names of SPARC and
X86 files,
e.g. have setup-x86 instead of setup-tftp-links. Is this appropriate?
131: same thing, although it seems less appropriate to make this change
here as
it could be less informative of a message.
setup-dhcp.sh:
109-110: This comment is misleading because it says Rootpath, but there
is only
rootpath and ROOTPATH and it is ROOTPATH that is meant. GRUBMENU is also
misspelled as GrubMenu.
Also, the header doesn't spell out that $sparc, DHTADM and TMP_DHCP are
assumed
to be set up before this routine is called.
Rather than have "sparc" made a global variable, I would prefer to see
it passed
in as an argument. Arguments make the code clearer than global variables.
147: Just checking: any need to restore original bootfile and rootpath?
setup-sparc.sh:
63: Just curious: why is this "touch" needed? A cp is done on the next line
anyway and I don't see anything gained by creating (or trying to create
for tet
purposes) the file before the cp.
85: I would plug in `uname -m` instead of hardwiring sun4v. It'll be
one less
thing to debug later, when trying different platforms.
118: A more useful error message would include the filename as $0 instead of
"Internal function to manage SPARC setup".
setup-tftp-links.sh: OK
prototype_com: OK
Thanks,
Jack
Susan Sohn wrote:
> Please review the changes for:
>
> 4194 need to make installadm tool changes for SPARC
> http://defect.opensolaris.org/bz/show_bug.cgi?id=4194
>
> which are posted at:
>
> http://cr.opensolaris.org/~sohn/4194
>
> Thanks,
> Sue
> _______________________________________________
> caiman-discuss mailing list
> caiman-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
>
