Sarah Jelinek wrote:
> 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?
I can answer this one since I did the prototype of this portion of the
code for Glenn. What he needed to do was remove the hardcoding of the
im-pop step and put it into a finalizer script. The
DC_execute_checkpoint function was only used to execute the checkpoint
in this one case. So when it disappeared so does this function.
jean
>
>
> -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
> _______________________________________________
> caiman-discuss mailing list
> caiman-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
