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
