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, OSTYPE, DIST_NAME and PKG_IMG_PATH. * All over the file: instead of assigning the return value $? to a variable and then call vmc_error_handler, I think you can just call the function with $? as an argument. usr/src/cmd/distro_const/vmc/export_vm: * lines 247-254: You can use "mkdir -m <mode> <dir_name>" to combine the mkdir followed by chmod usr/src/cmd/distro_const/vmc/install_vm * line 184-185: just curious, why do you assign the command to vbox_cmd and then, execute it in the next line, instead of execute it directly? usr/src/cmd/distro_const/vmc/post_install_vm_config * 194-198: I think it is better to check whether the RAM_SIZE specified is valid immediately after you get the valid, such as at line 175. I think it is kinda a waste of CPU power, to process all the other arguments just to find the RAM_SIZE is not correct. Likewise for CPU and VIRT_HW. * line 250: should the return value be checked? also, to make sure at least the empty string is not returned? * line 251-253: is it necessary to check and make sure the values assigned to all the orig_* variables are valid? usr/src/cmd/distro_const/vmc/prepare_ai_image * line 43, typo, prefix * line 95-99, nit, there's no need to invoke "rm" 5 times, you can just invoke it once with all the file names listed. * line 238-242, nit again, all the "rm" can be combined into 1 * line 246: forgot to remove this line? * line 248-end_of_file: instead of assigning cmd_stat to $?. I think you can directly pass $? to the vmc_error_handler function. usr/src/cmd/distro_const/vmc/vmc_common * line 44, typo, should be locale * line 120, wrong function name listed * line 125: the comment says input is none, but looks like at least 1 input is provided to this function. usr/src/cmd/distro_const/vmc/vmc_image.xml * Some previous discussion of code review comments mentioned that you will add a "flag post" to this file telling ppl what people most like will modify or not, but I don't see this in the file yet. * Also, I remembered that we will move the <distro_constr_flags> section to be in front of the <finalizer>section since distro_const_flags will more likely be modified, while the finalizer scripts sections is less likely to be modified. * For the "export-esx" and "export-ovf" checkpoints, do you mean the script name to be /usr/share/distro_const/vmc/export_vm? The 2 scripts listed are removed, right? Thanks, --Karen Glenn Lagasse wrote: > The following link will display the updated webrev for the VMC project. > It is a diff against what we submitted initially. > > <http://cr.opensolaris.org/~glagasse/webrev.diffs> > > We would like to get feedback on this round of changes no later than COB > this Friday if at all possible. If you would like to review these > changes but can't be done with your comments by then please ping me and > we'll work something out. > > CAVEATS: > > o All pep8/pylint comments not addressed in this webrev for anything > other than im_pop.py will be addressed when we merge with the python2.6 > work and pick up Jean's fixes. > > o Some of the diffs will show changes that occurred in slim_source > outside of VMC. This is the unfortunate result of my not reusing my > initial workspace to address the code review comments. I could go back > and do this if it's a real pain point to people (but would prefer not to > at this point). > > o There's a small change coming for prepare_ai_image due to the > bootroot/boot_archive changes in slim_source. That will be handled > before integration and sent out as a small seperate review (essentially > a 4-line change). > > o .image_info file creation. One of the review comments I received was > that .image_info creation wasn't optimal. If the file can't be created, > the DC build continues on even though the resulting media that needs the > .image_info file (liveCD at the moment) won't work. I was going to > clean this up in a generic way such that only media types that require > it actually generate it. I wasn't able to come up with a solution that > I liked in the time I allotted. Since VMC isn't regressing anything in > regards to the .image_info file I'm instead going to file a bug against > DC and fix this post-putback outside of VMC. > > Thanks, > > Team VMC (Joe and Glenn) > _______________________________________________ > caiman-discuss mailing list > caiman-discuss at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >
