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
