Hi Karen, * Karen Tung (Karen.Tung at Sun.COM) 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 for the comments. My responses are in-line. > 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"? That is correct. As currently delivered, the VMC project does not make any content changes to the bootable AI image passed to it (other than modifying the grub menu to boot the default manifest entry). A useful modification of course would be to allow VMC to modify the default AI client manifest on whatever bootable AI image is passed in with one that the user supplies. I'm currently planning to add this after the initial delivery. I'll also likely be adding in support to supply a custom AI client manifest during construction of the bootable AI image (it currently does not allow that either). > - 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. Well, the finalizer section is now part of the output_image tag which is part of the distro_constr_params tag. Previously, it was part of the img_params tag. > - 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'll let Joe respond to this. > - 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. I'll let Joe respond to this. > - Instead of just calling the virtual box commands, it would be better to > define > the full path to the command, and use that variable. I'll let Joe respond to this. > - 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? I'll let Joe respond to this. > - 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. I'll let Joe respond to this. > - 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? I'll let Joe respond to this. > 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"? This actually was an oversight. See my response to Jean and Sundar. Though I'm currently wondering if it might not be a better idea (than what I outlined to them) to deliver an empty nm_vm_img_params so that we can get some manifest validation (ie a manifest must have iehter nm_img_params or nm_vmimg_params). > - lines 294-305: why is this commented out? This is related to my comment for lines 57-67. I had planned to remove it completely. > 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? Actually I did not. I'll double check that. Do you suspect that it doesn't work? > 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? That is correct. It was going to add significant time to the VMC schedule to break this up as outlined in 6366 which I deemed out of scope for VMC since that's not what VMC needed. For the rest of your comments about the finalizer scripts, I'll let Joe respond as he primarily wrote all of that. > usr/src/cmd/distro_const/vmc/create_vm > > - line 238, 242: would be good to be more specific and put MB after the > numbers. > > - line 215: if it is not numeric, I think we should fail immediately > > - line 221: Also should fail immediately here. > > 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? > - 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. > > 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 > > - line 212: would be good to include the unit in the error > > - 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. > > 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" > > - Line 275-276: the second sentense of this comment doesn't seem to > belong there. > > - 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. > > - line 453: should the message say, "Warning: failed to create the ISO"? > > - 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. > > - 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. > > 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 Agreed and I'll make that change. Thanks Karen! -- Glenn
