Huge thank you Jack. I worked on the finalazer ;) scripts. My comments on them are below in-line.
I will let Glenn comment on the other files. Joe Jack Schwartz wrote: > Hi Glenn and Joe. > > Congratulations on getting this out; it is quite an undertaking. It > looks pretty good. > > I've reviewed DC files and one VM-related file. I did not review > export_esx, export_ovf, install_vm, install_vm_config, prepare_ai_image. > > Here are my comments: > > DC-manifest.defval.xml: > > 138-142: Nit: move up to where other distro_const_params/... items are > (for example, just below 113) > > Regarding a comment which Karen asked: > >> - 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? >> > > You can run ManifestServ with the verbose option to check the > processing, and can then check at the values. -t dumps out the > temporary file after defaults have been added to it. Using the schema > called /tmp/DC-manifest.rng and defval manifest called > /tmp/DC-manifest.defval.xml, and a manifest called slim_cd_x86.xml, > the command would be: > > ManifestServ -v -t -f /tmp/DC-manifest slim_cd_x86.xml > > DC-manifest.rng: > > 64-67, 299-304: I saw the comments about this to other reviewers... I > would remove the comments, remove the <optional> on 59 and 62 instead > of the current approach, in order to keep the file general and > extensible. It will be easier to add new image types to this file if > the <choice> of 57-67 were left in. > > When you uncomment 299-304, I believe you can replace the <interleave> > lines with <empty/> if there is really nothing to add under vmimg_params. > > Glenn, you made a comment that if img_params was missing then the > build will fail. I don't think it should fail if the schema is set up > as above using "choice"; the other "choice" of "vmimg_params" is > being taken. Please ping me off line about this more if needed. > > DC_checkpoint.py: > > 722, 724: contains refs to DC_execute_checkpoint() which is now gone. > > im_pop.py: > > 76: I don't think flush is needed right before the close. The close > should do an implicit flush. > > a list of authorities to cleanup is created in 281. What about > exceptions raised between 281 and 454, where cleanup occurs? > > Close pkgfile before using it. Move 404 behind 401. In fact, I think > you can get rid of the flush if you close it anyway. > > Where do logging messages go from TM now? > > 136, other places: line > 80 chars > ... Others have already said that the py files are not PEP8 compliant > so I won't dwell on these things. > > In general, it is best to break up methods which are more than a few > screens in height. This makes the code more clear and digestable. > > 357: It makes more sense and is cleaner from an OS perspective to > close the file rather than flush here and close later. The transfer > module (called via dc_ips_contents_verify on 360) will just open > the file again anyway. This will also condense several closes which > are called during error handling into just the one on 357. > Alternatively, if there is some reason why the file cannot be closed > as I suggest, please document the reason in the code. > > 398: ditto. I suggest moving the close() of 404 to 398, replacing the > flush(). > > create_vm: > > I second Karen's suggestion to use a full path to commands. This makes > what is being run explicit and handles the case where the $PATH isn't > set properly. Other distro-constructor scripts also do this. I tend to use full paths to commands but setting PATH is a good idea in case a command is missed. Doing so was initially suggested to me 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 I will be improve the comments around setting PATH. > > 130: Printing "FAILURE" after printing "Invoking: error_handler" may > confuse the user to believe that the error handler failed. I would > say "Failure: Invoking error_handler" before cleanup, and no message > after cleanup unless the cleanup itself failed. Good suggestion. - Will do. > > 215, 221, other places: I thought single line if/then/fi was > discouraged. The shell style guide says it shell should look like C, > and does not itself use single line examples for if/then/fi. I looked > through Cstyle though, and couldn't find the admonishment for doing > this, so not sure... I will be changing these. > > 256: typo finalazer -> finalizer Good catch! Thanks. > > prototype_com: OK > all Makefiles: OK > all XML except as noted above: OK > all py files except as noted above: OK notwithstanding comments from > others > > Thanks, > Jack
