Hi Glenn and Team,
Great work getting this done! I do have a few comments(don't I always
:-))....
If I repeated anything other folks said, just point me to your responses
to them.
DC-manifest.defval.xml:
-You added the 'skip_if_no_exist' to all of the default values. Is there
a way to set a global macro or something to do this, so we don't have
to add it to every line? Perhaps in DC_defs.py? Not quite sure if this
will work, just checking.
-Also, I assume "img_params", if not present for standard DC runs will
result in a different failure, not a skip_if_not_present? Wondering what
happens if this node path definition isn't present for DC runs.
-why did you move/rename the img_params, and distro_const_params? For
example, you included the build_area under the distro_const_params now.
Just wondering.
DC_checkpoint.py:
-Why did you remove the DC_execute_checkpoint method in this file? I
didn't see this in another module. Did I miss it?
-prepare_ai_image:
> mkdir ${MNT_ISO}; cmd_stat=$?; [[ ${cmd_stat} != 0 ]];
> mkdir_stat=${cmd_stat}
> 279 mkdir ${MNT_MROOT}; cmd_stat=$?; [[ ${cmd_stat} != 0 ]];
> mkdir_stat=${cmd_stat}
> 280 mkdir ${TMP_ISO}; cmd_stat=$?; [[ ${cmd_stat} != 0 ]];
> mkdir_stat=${cmd_stat}
> 281 mkdir ${SCRATCH}; cmd_stat=$?; [[ ${cmd_stat} != 0 ]];
> mkdir_stat=${cmd_stat}
> 282
> 283 if [[ ${mkdir_stat} != 0 ]] ; then
> 284 print -u2 "\nWarning: Failed to make a needed directory: " \
> 285 "${MNT_ISO} or ${MNT_MROOT} or ${TMP_ISO} or " \
> 286 "${SCRATCH}"
> 287 error_handler
> 288 fi
This code allows for repeated failures before printing the warning,
right? And, the warning message is ambiguous in terms of what really
failed. Can we make this more crisp by adding error checking at each point?
> 211 shift # This script does not need <rsock>
> 212 typeset -r PKG_IMG_PATH="$1"
> 213 typeset -r TMP_DIR="$2"
> 214 shift # This script does not need <br_ba>
> 215 shift # This script does not need <media_out>
> 216 typeset -r ISO_FILE="$3" # path to bootable ai image
Doesn't the last 'shift' reset the next argument to $1? That's how I am
reading the man page, but I could be misunderstanding it.
> 325 if [[ -f ${MNT_ISO}/boot/x86.microroot ]] ; then
> 326 cp ${MNT_ISO}/boot/x86.microroot ${SCRATCH}/x86.microroot.gz
> 327 /usr/bin/gunzip ${SCRATCH}/x86.microroot.gz
Line 327, we should check either the outcome of the gunzip call or that
the file exists before calling gunzip.
> 431 else
> 432 print -u2 "\nWarning: GRUB menu.lst file found."
> 433 error_handler
The warning should say 'GRUB menu.st file not found'.
post_install_vm_config:
> 67 cleanup ()
> 68 {
> 69
> 70 #
> 71 # It is not necessary to process errors.
> 72 #
> 73 {
> 74 trap "" ERR INT
> 75 set +o errexit
> 76
> 77 #
> 78 # Attempt to reconfigure the VM to it's original
> settings.
> 79 #
> 80 VBoxManage -q modifyvm ${DIST_NAME} --memory
> ${orig_ram_size} \
> 81 --cpus ${orig_cpus} --hwvirtex ${orig_virt_hw}
> 82
> 83 } > /dev/null 2>&1
Do we need to capture any errors here, rather than piping them to /dev/null?
In general in this script I see piping to /dev/null.. wouldn't it be
better to capture this to a log or something?
General comment about scripts:
-Couldn't we create a method that handles this error:
344 cmdsts=$?
345 if [[ ${cmdsts} != 0 ]] ; then
346 print -u2 "\nError: modifyvm ${DIST_NAME} failed."
347 error_handler
348 fi
We have this if statement in all scripts. Maybe error_handler can
include the check for $cmdstats and do the right thing?
thanks,
sarah
****
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.
install_vm:
-piping output of commands to /dev/null. Same as above comment.
> 178 shift # This script does not need <pkg_ia>
> 179 shift # This script does not need <tmp_dir>
> 180 shift # This script does not need <br_ba>
> 181 shift # This script does not need <media_out>
Not sure these shifts are necessary. I don't see you using any $1 later
in the code. Or $2, etc...
In the case of checking the vbox version, in this script and in the one
I comment on above here:
> 214 typeset -i vbox_ver=`VBoxManage -v | sed 's/\..*//'`
> 215 if [[ ${vbox_ver} < 3 ]] ; then
> 216 print -u2 "\nError: VirtualBox must be at least"\
> 217 "version 3. The version installed is: `VBoxManage -v`"
> 218 exit 1
> 219 fi
Do we want to call error_handler() instead of exit 1? If not, why?
> 263 vm_disk=`VBoxManage -q showvminfo ${DIST_NAME} | grep "^Primary master:"
> | \
> 264 awk '{print $3}'`
Do we need to assign vm_disk in this case? I can't see where it is used
after that.
>
> The timeout for providing review comments is COB October 23rd, 2009. If
> you need more time, please let me know.
>
> Thanks!
>
>
>
> ------------------------------------------------------------------------
>
> _______________________________________________
> caiman-discuss mailing list
> caiman-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
