Jack, > Round 2. >>> >>> create-client.sh: >>> 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>". > So the check at 254 for an empty $BOOT_FILE variable isn't needed.
The check on 254 is checking for a non-empty BOOT_FILE to indicate that the user has used the -f option. If it is sparc, they get an error message. I have added a comment to clarify. >>> 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. > All the more reason to correct the comment, assuming the comment really > refers to $ROOTPATH and $GRUBMENU referenced in the last if/else/fi in > the routine. Rootpath and GrubMenu aren't even referred to in the routine. I have modified the comment to make it clearer. >>> 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. > I respectfully disagree. I suggested this because globals can be > modified under the covers; I think it is good to spell out the > environment required for a routine to execute unless it is obvious. > That said, it's only a suggestion. >> >>> 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 have changed sparc so that it is no longer a global and is now passed as an argument. ;) webrev updated at: http://cr.opensolaris.org/~sohn/4194 Sue
