Joe,
Your responses are fine with me. I will go through the code again
when you and Glenn send out the updated version.
Thanks,
Sundar
Joseph J. VLcek wrote:
> Huge Thank you for the code review comments! Sundar!
>
> I worked on the finalizer scripts and will respond to your comments
> for those files in line below.
>
> Joe
>
> sundar Yamunachari wrote:
>> Glenn Lagasse wrote:
>>> The VMC project is proud to present it's code for your review. You may
>>> access the webrev at:
>>>
>>> http://cr.opensolaris.org/~glagasse/slim_vmc/
>>>
>>> I'm also attaching the DC log file for a VMC run as well as the console
>>> output for the same run for the curious as to what this looks like in
>>> practice.
>>>
>>> The timeout for providing review comments is COB October 23rd,
>>> 2009. If
>>> you need more time, please let me know.
>>>
>>> Thanks!
>>>
>>>
>>> ------------------------------------------------------------------------
>>>
>>>
>>> _______________________________________________
>>> caiman-discuss mailing list
>>> caiman-discuss at opensolaris.org
>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
>> Glenn,
>>
>> usr/src/cmd/distro_const/DC-manifest.rng:
>>
>> Is there a reason why vmc fields are commented in the schema files
>> (lines 65-67, 299-305). Are these definitions are not used or not
>> needed?
>>
>> DC_defs.py:
>>
>> You got me confused here. Changing DISTRO_PARAMS -> IMG_PARAMS and
>> IMG_PARAMS->DISTRO_PARAMS ;-)
>>
>> usr/src/cmd/distro_const/utils/im_pop.py:
>>
>> General comments: The pkg set-authority is changed to set-publisher.
>> Do we need to convert authority to published for this project?
>>
>> 80: Why you have a 'pass' here?
>> 107: Can you add some comments to the function 'dc_ips_unset_auth'
>> 180, 188, 197, 208, 220: Comments the functions in those lines
>> 315: publisher is used here and authority is used in all other places
>>
>> usr/src/cmd/distro_const/vmc/create_vm:
>>
>> 201: The error message could be more descriptive with usage
>
> I had initially provided a "usage" function for all of the finalizer
> scripts. Glenn pointed out that the finalizer scripts do not run from
> the command line so a "usage" could be confusing the a user. I agree
> with Glenn now.
>
> That said improving this error message is a good idea.
>
>
>> 215: If DISKSIZE is non-numeric, why you are continuing?
>> 221: same comment as in line 215 for RAM_SIZE
>
>
> The DISKSIZE and RAM_SIZE errors are detected and reported in the
> section of code below that validates the input arguments.
> Lines 236->246
>
>
>> 238: Add the unit to the message like 12000 MB
>> 244: Same as the above comment
>
> good point. Will do.
>>
>> usr/src/cmd/distro_const/vmc/export_esx:
>>
>>
>> 170: The comments indicate 7 args passed to this script but the line
>> checks only for 5 args?
>
> good catch. The comment needs to be updated.
>
>
>> 218-233: They are same here and create_vm. Is it possible to use only
>> one copy?
>
> I could investigate creating a single function to do this into a
> common file.
>
>
>>
>> usr/src/cmd/distro_const/vmc/export_ovf:
>>
>> 170: The comments indicate 7 args passed to this script but the line
>> checks only for 5 args?
>
> good catch. The comment needs to be updated.
>
>
>> 190: Extra '=' in ${MEDIA=}
>
> good catch!
>
>
>>
>> usr/src/cmd/distro_const/vmc/post_install_vm_config:
>>
>> 182: The error message could be more descriptive with usage
>
>
> I had initially provided a "usage" function for all of the finalizer
> scripts. Glenn pointed out that the finalizer scripts do not run from
> the command line so a "usage" could be confusing the a user. I agree
> with Glenn now.
>
> That said improving this error message is a good idea.
>
>
>> 196: If RAM_SIZE is non-numeric, why you are continuing?
>
> This section of code captures the input arguments. The argument
> validation is preformed below lines 208.
>
>
>> 199: RAM_SIZE -->CPUS
>
> Good catch.
>
>
>> 202: same comment as in line 196 for CPUs
>
> Same answer as comment in line 196 ;)
>
>
>> 212: Add the unit to the message like 12000 MB
>> 218: Same as the above comment
>
> Will do
>
>
>>
>> - Sundar
>
>
> Thanks for the help Sundar!
>
> Joe
