Hi Jack, Thank you for the review. Responses below.
Jack Schwartz wrote: > 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 Done. > 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. You are correct in that -f is invalid for sparc. If -f is specified, the parsing code at 180 verifies that there is a BOOT_FILE specified. Thus we know that if we've gotten to 254, the user has specified "-f <boot_file>". > 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. I think this is a good idea, but I don't think that I can implement this and then regression test the changes in time. I would like to write a high priority bug on this for now and will look at making this change soon. > install-common.sh: > > ~159: Please add a comment that create_menu_lst_file is for x86. ok > 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. Changed to do so. > Being new to this code, I'll ask: why is create_menu_lst_file called > from both > create_client and setup-tftp-links? setup-tftp-links is called by create-service so there is a separate call. > 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. Those declarations were not added by these code changes. I would prefer not to change them. > 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) Same as above. > 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? It might not be appropriate because although setup-tftp-links is only used for x86 right now, that might not always be the case in the future. > 131: same thing, although it seems less appropriate to make this change > here as > it could be less informative of a message. I agree, so I'll leave it as is. > 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. Rootpath and GrubMenu are the actual dhcp macro symbol names. You can see the definitions at the top of the file. > Also, the header doesn't spell out that $sparc, DHTADM and TMP_DHCP are > assumed > to be set up before this routine is called. I don't think that is necessary as they are global. > 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. I would prefer not to do this. I actually considered passing it but thought it was cleaner with the global. > 147: Just checking: any need to restore original bootfile and rootpath? No need. > 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. Agreed. Changed to use mv per Jan's comment. > 85: I would plug in `uname -m` instead of hardwiring sun4v. It'll be > one less > thing to debug later, when trying different platforms. uname -m would provide information about the server. We need to know the information about the client. It is hardcoded for now. If it is decided to support more than sun4v, we will need to add a command line option for the user to provide the platform. > 118: A more useful error message would include the filename as $0 > instead of > "Internal function to manage SPARC setup". This was done ala the other setup-* functions, so will leave as is for now. Webrev is updated, same location. Sue > 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 >> >
