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.
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.
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...
256: typo finalazer -> finalizer
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
