Thanks Sarah!
My comments regarding the scripts below.
Joe
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?
>
>
> -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?
Good point. I will provide individual messages. See comments below about
streamlining the error checking/error_handling code.
>
>> 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.
The man page is misleading. I made the same mistake.
The shift code is correct. ISO_FILE is the 6th argument. I issue 3
shifts and then pull the value from $3 (3 shifts + arg 3 == 6)
shift # This script does not need <rsock>
typeset -r PKG_IMG_PATH="$1"
PKG_IMG_PATH is actually in $2 but minus the one shift puts that in $1
typeset -r TMP_DIR="$2"
TMP_DIR is actually in $3 but minus the one shift puts that in $2
shift # This script does not need <br_ba>
shift # This script does not need <media_out>
typeset -r ISO_FILE="$3" # path to bootable ai image
ISO_FILE is actually in $6 minus the 3 shifts make it available at $3
>> 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.
Good point. Will do.
>> 431 else
>> 432 print -u2 "\nWarning: GRUB menu.lst file found."
>> 433 error_handler
>
> The warning should say 'GRUB menu.st file not found'.
Well yup. That would make sense! ;) Good catch. Thanks
>
> 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?
I learned to do this by Roland Mainz (Mr. bash) when I was working on
usbgen.
If the cleanup routine is invoked because an error has occurred it is
likely the error_handler may generate errors too. The error messages
coming from the cleanup routine could be misleading to the user and not
direct them to the initial problem.
>
> 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
I agree it would be cleaner to update the error_handler routine to
invoked with:
error_handler( cmdsts, \nError: modifyvm ${DIST_NAME} failed." )
The "pseudo" code would look like:
error_handler
{
typeset -i cmdsts=$1
typeset message="$2"
if [[ ${cmdsts} == 0 ]] ; then
# do nothing the cmdsts is success.
return
fi
print -u2 "${message}"
...
# process the remaining error handler code...
}
>
> 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.
See my comment above
>> 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...
Good catch. These shifts are no longer needed.
>
> 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?
Good point. I will invoke error_handler here too.
>
>> 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.
>
Good catch!
vm_disk should actually be used in the error handler. It seems to have
been inadvertently removed from there.
>
>
>
>
>
>
>>
>> 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
