Joseph J. VLcek wrote:
> Karen Tung wrote:
>> Hi Joe,
>>
>> I have comment about 1 of your response in-line below. I removed
>> everything
>> else that I have no further comment on.
>>
>> Joseph J. VLcek wrote:
>>>
>>> Hi Karen,
>>>
>>> Thank you for the feedback. My response are in-line below.
>>>
>>> Thank you!
>>>
>>> Joe
>>>
>>> Karen Tung wrote:
>>>> Hi Glenn and Joe,
>>>>
>>>> Here are my comments on the updated webrev.
>>>>
>>>> usr/src/cmd/distro_const/vmc/create_vm:
>>>> * lines 233-237: I think it is better to validate the value specified
>>>> for disk_size immediately after you get the value in line 201.
>>>> That way, we don't waste CPU power to get the other values if
>>>> the disk_size is not valid. Same comment applies to the ram size,
>>>
>>> I feel a very minimal amount of CPU would be saved by combining the
>>> parameter gathering with the parameter validation. I also fell doing
>>> so would reduce the readability and therefor the maintainability.
>>>
>>> I would like to leave this as it is.
>> I suppose it is up to you. Personally, wasting CPU power or not aside,
>> I think the code is easier to read if all the validation for the same
>> supplied
>> argument happens in the same place instead of scattered all over.
>>
>> Thanks,
>>
>> --Karen
> Hi Karen.
>
> I agree validation should be done in one place. I planned to have it
> all under the "Validation section". One exception ended up making
> sence. That being the validation of INTEGER.
>
>
> I want to assign the INTEGER values to "integer" variables using the
> "-i" flag to typeset.
>
> If the command line does not contain an integer when one is expected
> the typset -i results in the variable having the value "0".
>
> e.g.:
> % typeset -i int_variable=1 % echo $int_variable 1
> % typeset -i int_variable="hi"
> % echo $int_variable
> 0
> %
>
> I only confirm it is an integer prior to assigning it with "typeset
> -i". The range validation is still done under the "Validation section"
>
> Because of this I felt this exception to where input arguments are
> validated would be acceptable.
>
>
>
> A way to have no exception to input validation all happening under the
> "Validation section" would be to use 2 variables.
>
> e.g.:
>
> Instead of this:
>
> 194 if [[ $6 != +([0-9]) ]]
> 195 then
> 196 print -u2 "\nImproper argument."
> 197 print -u2 "\"disk size\" must be an integer, min \"12000\"
> (MB)."
> 198 exit 1
> 199 fi
> 200 typeset -i DISK_SIZE=$6 # VM Disk size
>
>
> I could do this:
>
> typeset TMP_DISK_SIZE=$6 # VM Disk size <- Note no "-i" flag to typeset.
>
> Then under validation I could do:
> 194 if [[ ${TMP_DISK_SIZE} != +([0-9]) ]]
> 195 then
> 196 print -u2 "\nImproper argument."
> 197 print -u2 "\"disk size\" must be an integer, min \"12000\"
> (MB)."
> 198 exit 1
> 199 fi
> 200 typeset -i DISK_SIZE=${TMP_DISK_SIZE} # VM Disk size
>
> 233 if ((${DISK_SIZE} < 12000 || ${DISK_SIZE} > 99999999)) ; then
> 234 print -u2 "\nThe disk size of ${DISK_SIZE} is invalid. It
> must" \
> 235 " be in range [12000, 99999999] (MB)"
> 236 exit 1
> 237 fi
>
>
> This seemed a bit overkill to me for the one exception.
>
> I would like to leave this the way it is. I suspect that is OK with
> you since you have already written: "I suppose it is up to you. " but
> I thought I would try to explain why it is the way it is.
>
> What do you think?
>
> Joe
>
Karen and I have discussed this off line. The agreed upon solution is
the one I proposed above using the two variables.
So I will change the code to do that.
Thank you for your help Karen.
Joe
