Hi Sue.
On 01/08/09 10:15, Susan Sohn wrote:
> 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.
>
OK, but the clearest thing would be just to modify around 180.
something like:
-f) BOOT_FILE=$2
if [ ! "$BOOT_FILE" ] ; then
usage;
else if ["${IMAGE_TYPE}" = "${SPARC_IMAGE}" ] then
echo "${myname}: \"-f\" is an invalid option for SPARC"
usage;
endif
>
>>>> 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.
>
Thanks. That really helps.
>
>>>> 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. ;)
>
Looks good here too.
Thanks,
Jack
> webrev updated at:
> http://cr.opensolaris.org/~sohn/4194
>
> Sue
> _______________________________________________
> caiman-discuss mailing list
> caiman-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://mail.opensolaris.org/pipermail/caiman-discuss/attachments/20090108/1386f442/attachment.html>
