Hi Sue.
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.
>
>> 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.
OK
>> 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.
>
>> 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 would prefer not to do this. I actually considered passing it but
> thought it was cleaner with the global.
>> setup-sparc.sh:
>> 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.
Oh... oops. OK.
>
>> 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.
OK, I guess.
Thanks,
Jack
>
> 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
>>>
>>
>
