[re-sending since I haven't seen my first mail hit the list or archives] Hi Jack,
Thanks for the review! Responses in-line. * Jack Schwartz (Jack.A.Schwartz at Sun.COM) 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) Accepted. > 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 Excellent. That's quite useful. Thanks Jack. > 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. Accepted. > 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. I agree. > 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. Sure. Once I add in the vmimg_params support, then manifests must have either vmimg_params *or* img_params. So, we're covered. > DC_checkpoint.py: > > 722, 724: contains refs to DC_execute_checkpoint() which is now gone. Accepted. Will remove the refs. > im_pop.py: > > 76: I don't think flush is needed right before the close. The close > should do an implicit flush. This is existing code from distro_const.py which I've just moved. That said, I've no qualms about changing this, so I'll do so when I refactor the image_info creation as part of other comments related to it. > a list of authorities to cleanup is created in 281. What about > exceptions raised between 281 and 454, where cleanup occurs? This is existing code (from DC_tm.py). It would appear that there are places where exceptions are raised and thus authority cleanup doesn't happen. Since this is existing code that's merely moving locations I'm leery of changing it to drastically at this point. I can however file a bug if that's acceptable. > 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. Accepted. > Where do logging messages go from TM now? I'm not sure I understand the question. The same place they always did, the DC log files. > 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. Accepted. > 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. This is existing code, that I didn't write (DC_tm.py). This is another case of I'd rather not change things too drastically that I haven't touched. > 398: ditto. I suggest moving the close() of 404 to 398, replacing > the flush(). Accepted. As for the rest of your comments, Joe has responded to them I believe. Thanks for looking at this Jack. It's much appreciated. Glenn
