Karen, Thank you for the feedback. My comments are below regarding the new VM finalizer scripts, as that's the piece I worked on. I'll let Glenn comment on the other files.
Joe Karen Tung 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! >> >> > Hi Glenn and Joe, > > Attached are my code review comments. If I repeated comments others > have made, please > just point me to your response to them. > > Thanks, > > --Karen > > > ------------------------------------------------------------------------ > > _______________________________________________ > caiman-discuss mailing list > caiman-discuss at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/caiman-discuss > > General comments: > --------------------- > - Question: Is it true that when this project integrates as it is coded > now, users will not be able to specify what is included in the resulting > VM image? The content of the VM image is dictated by whatever is > the content of the default AI manifest? Since the default AI manifest > installs from http://pkg.opensolaris.org/dev, the only way to > change the repo from which the bootable AI install is done is to > generate my own AI image with a different default AI manifest? > If that's the case, what's the plan to support specifying what > I want to include in the VM image and from where I want the AI install > to happen "easily"? > > - All the 4 existing manifests - The list of finalizer scripts used > to be in the position of the file below the warning about not > modifying certain fields in the manifest. They are now moved > to a spot before that warning. That might confuse people. > I think it is better to move them back to be below the warning. > > - In all the new finalizer scripts, the comment about setting the PATH > says this script Solaris needs the POSIX-conforming command. Why is that? > If something in the script really needs POSIX conforming command, > it would be good to know exactly what it is. I learned to do this by Roland Mainz (Mr. bash) when I was working on usbgen. It is in our ksh93 tips http://installzone-wiki.central.sun.com/wiki/index.php/Ksh93_Tips Setting PATH will ensure consistent the same commands are used regardless of the environment they are run in. This will ensure consistent behavior. > > - In many of the new finalizer scripts, you are printing the command > that will be executed. Currently, you have the command in the print > statement, then, hardcoded the command to be executed. This is not > the optimal way to do it. In the future, if you happen to modify > the command to be executed, you will have to change both. There might > be chances that you change one without the other. So, the debugging > output might or might not be correct. One suggestion I have is to > assign the command to a variable, then, you print the value of that > variable, and then "exec ${cmd}". This way, you will know exactly > what command with what arguments you are executing. The messages were there to help with development. We are going to change the messages so they are not precisely the command which will be executed but instead better communicate progress to the user. This will lead to the user experience staying much the same when the underlying functionality is rewritten in Python. > > - Instead of just calling the virtual box commands, it would be better to define > the full path to the command, and use that variable. Agreed. Will do. > > - Would it make sense to put the segment of code in all the new > finalizer scripts that sets the C locale into a library or something > so we don't have the exact same code all over the place? Yes. Agreed. Will do. > - The code to check whether VirtualBox is installed and whether it is > at least version 3 appears in all finalizer scripts. I think it is > also better to put them into functions in a library. The code to > check on whether VirtualBox is running is also duplicated in many > files. That can also be a function. Yes. Agreed. Will do. > > - In all the new finalizer scripts, you are doing "shift" for all the unused > arguments. I don't think doing shift is needed. You know exactly > which argument you will need, and exactly what their position is, why > not just refer to them directly? Agreed. And using the shift has actually created some confusion as to which argument is where. I will remove the shifts. > > Specific Comments: > -------------------- > > usr/src/cmd/distro_const/DC-manifest.rng > > - line 57-67: why is the nm_img_params optional? Shouldn't it be either > nm_img_params or nm_vming_params? > > - Would be more clear to name the 2 choices as "im_live_img_params" and > "nm_vm_img_params"? > > - lines 294-305: why is this commented out? > > usr/src/cmd/distro_const/DC-manifest.defval.xml > > - Did you verified that the appropriate default value > is assigned after you added all the skip_if_no_exist things? > For example, if I have a manifest that have the img_params section, > but I didn't specify some of the values in the manifest, > did the "right" value get assigned? > > usr/src/cmd/distro_const/utils/im_pop.py > > - This just moves the "original" im_pop step into a finalizer script. > The bug (6366) requests for breaking up the im_pop step. Does this mean > your changes will not address the bug? > > usr/src/cmd/distro_const/vmc/create_vm > > - line 238, 242: would be good to be more specific and put MB after the numbers. Agreed. The input does not contain the scale. If the input is incorrect and the error message include the scale the user may think the scale is required. I will reword the message to include scale and make it clear scale is not accepted in the input. > > - line 215: if it is not numeric, I think we should fail immediately Agreed. Will do. > > - line 221: Also should fail immediately here. Agreed. Will do. > > usr/src/cmd/distro_const/vmc/export_esx > usr/src/cmd/distro_const/vmc/export_ovf > > - line 189 of the above 2 files: is the '=' after MEDIA_OUT intentional? Not intentional. Good catch. > - Many sections of these 2 scripts are identical, please put the identical > into a common function that both script use, so we don't have duplicate code. These two scripts will be merged into one which will accept an argument to identify which format the output should be. > > usr/src/cmd/distro_const/vmc/post_install_vm_config > > - line 196, 202: if the arguments is not what you expect, should just fail > immediately instead of continuing Agreed. Will do > > - line 212: would be good to include the unit in the error Agreed. See above > > - line 222: here, you check for "on" or "off". However, in the > manifest, the comment says virtual machine VT-x/AMD-V support can be either > on, off or default. The comment in the manifest should not include "default". > > usr/src/cmd/distro_const/vmc/prepare_ai_image > > - line 96-114: is it really necessary to check whther those > files/directories exist? I think it will be OK to just do "rm -rf" No. OK will remove the checks. > > - Line 275-276: the second sentense of this comment doesn't seem to > belong there. I think the first sentence should go and the 2nd stay. I think this will satisfy your point of the redundancy. > > - lines 278-288: why not fail immediately when the return code > for one of the mkdir calls != 0. I don't see any point > in having to save the return code, and continuing, just to exit later. Agreed. Will do. > > - line 453: should the message say, "Warning: failed to create the ISO"? Good catch! > > - lines 319-358: it would be better to move this block of code to be > after the GRUB menu modifications section, because the GRUB menu modifications > is not using any value derived by this code. This is code is unnecessary > executed if the GRUB menu modification failed for some reason. Agreed. Will do > > - The modified bootable AI iso is placed in "$PKG_IMG_PATH}/vmc_modified.iso" > Other scripts makes assumption about this name and location too. > I think it is better to have this be a constant defined in a library somewhere. Agreed. Will do. > > usr/src/cmd/distro_const/vmc/vmc_image.xml: > > - For all the arguments passed into the scripts, it would be useful to > add in the comment what the unit is for the varies sizes Will do. Huge Thanks! Joe
