Hi Karen,
Thank you for the feedback. My response are in-line below.
Thank you!
Joe
Karen Tung wrote:
> Hi Glenn and Joe,
>
> Here are my comments on the updated webrev.
>
> usr/src/cmd/distro_const/vmc/create_vm:
> * lines 233-237: I think it is better to validate the value specified
> for disk_size immediately after you get the value in line 201.
> That way, we don't waste CPU power to get the other values if
> the disk_size is not valid. Same comment applies to the ram size,
I feel a very minimal amount of CPU would be saved by combining the
parameter gathering with the parameter validation. I also fell doing so
would reduce the readability and therefor the maintainability.
I would like to leave this as it is.
> OSTYPE, DIST_NAME and PKG_IMG_PATH.
> * All over the file: instead of assigning the return value $? to a
> variable
> and then call vmc_error_handler, I think you can just call the function
> with $? as an argument.
You are correct, many places in the code do not need the assignment of
$? to a status variable. However I don't feel doing so will noticeably
detract from the performance.
I have chased a few shell script bugs where a subsequent command had
alter $? leading to the wrong command status being evaluated. So I
developed the practice of saving $? into a status variable. I still feel
it is safe and not costly.
I would like to leave this as it is also.
>
> usr/src/cmd/distro_const/vmc/export_vm:
> * lines 247-254: You can use "mkdir -m <mode> <dir_name>" to combine
> the mkdir followed by chmod
Good suggestion. Will do.
>
> usr/src/cmd/distro_const/vmc/install_vm
> * line 184-185: just curious, why do you assign the command to
> vbox_cmd and then, execute it in
> the next line, instead of execute it directly?
Because vbox_cmd is used on line 187, in the error message also. It
should also be used in the log message.
I will change this from:
183 print -u1 "\nInvoking: VBoxHeadless startvm ${DIST_NAME}"
184 vbox_cmd="VBoxHeadless -startvm ${DIST_NAME}"
185 ${vbox_cmd}
186 cmdsts=$?
187 vmc_error_handler ${cmdsts} "\nError: ${vbox_cmd} failed"
To:
vbox_cmd="VBoxHeadless -startvm ${DIST_NAME}"
print -u1 "\nInvoking: ${vbox_cmd}"
{vbox_cmd}
cmdsts=$?
vmc_error_handler ${cmdsts} "\nError: ${vbox_cmd} failed"
>
> usr/src/cmd/distro_const/vmc/post_install_vm_config
> * 194-198: I think it is better to check whether
> the RAM_SIZE specified is valid immediately after
> you get the valid, such as at line 175. I think it is
> kinda a waste of CPU power, to process all the other arguments
> just to find the RAM_SIZE is not correct. Likewise for CPU and VIRT_HW.
see my reply to this issue as raised for file create_vm
> * line 250: should the return value be checked? also, to make sure at
> least the
> empty string is not returned?
That would be caught above at the check if the VM is running. I don't
think it is necessary to do it again.
> * line 251-253: is it necessary to check and make sure the values
> assigned to
> all the orig_* variables are valid?
I don't thing so because whatever value Virtual Box returns will simply
be reapplied to the virtual machine in the exit handler.
>
> usr/src/cmd/distro_const/vmc/prepare_ai_image
> * line 43, typo, prefix
Good catch.
> * line 95-99, nit, there's no need to invoke "rm" 5 times, you can just
> invoke it once with all the file names listed.
> * line 238-242, nit again, all the "rm" can be combined into 1
I will change these
> * line 246: forgot to remove this line?
Yes, good catch.
> * line 248-end_of_file: instead of assigning cmd_stat to $?. I think
> you can directly
> pass $? to the vmc_error_handler function.
see my reply to this issue as raised for file create_vm
>
> usr/src/cmd/distro_const/vmc/vmc_common
> * line 44, typo, should be locale
Good catch.
> * line 120, wrong function name listed
Good catch.
> * line 125: the comment says input is none, but looks like at least 1
> input is provided to this function.
Good catch.
>
> usr/src/cmd/distro_const/vmc/vmc_image.xml
I will let Glenn reply to this.
> * Some previous discussion of code review comments mentioned
> that you will add a "flag post" to this file telling ppl what people
> most like will modify or not, but I don't see this in the file yet.
> * Also, I remembered that we will move the <distro_constr_flags>
> section to be in front of the <finalizer>section since
> distro_const_flags will more likely be modified, while the finalizer
> scripts
> sections is less likely to be modified.
> * For the "export-esx" and "export-ovf" checkpoints, do you mean the
> script name to be /usr/share/distro_const/vmc/export_vm? The 2 scripts
> listed are removed, right?
>
> Thanks,
>
> --Karen
>
>
> Glenn Lagasse wrote:
>> The following link will display the updated webrev for the VMC project.
>> It is a diff against what we submitted initially.
>>
>> <http://cr.opensolaris.org/~glagasse/webrev.diffs>
>>
>> We would like to get feedback on this round of changes no later than COB
>> this Friday if at all possible. If you would like to review these
>> changes but can't be done with your comments by then please ping me and
>> we'll work something out.
>>
>> CAVEATS:
>>
>> o All pep8/pylint comments not addressed in this webrev for anything
>> other than im_pop.py will be addressed when we merge with the python2.6
>> work and pick up Jean's fixes.
>>
>> o Some of the diffs will show changes that occurred in slim_source
>> outside of VMC. This is the unfortunate result of my not reusing my
>> initial workspace to address the code review comments. I could go back
>> and do this if it's a real pain point to people (but would prefer not to
>> at this point).
>>
>> o There's a small change coming for prepare_ai_image due to the
>> bootroot/boot_archive changes in slim_source. That will be handled
>> before integration and sent out as a small seperate review (essentially
>> a 4-line change).
>>
>> o .image_info file creation. One of the review comments I received was
>> that .image_info creation wasn't optimal. If the file can't be created,
>> the DC build continues on even though the resulting media that needs the
>> .image_info file (liveCD at the moment) won't work. I was going to
>> clean this up in a generic way such that only media types that require
>> it actually generate it. I wasn't able to come up with a solution that
>> I liked in the time I allotted. Since VMC isn't regressing anything in
>> regards to the .image_info file I'm instead going to file a bug against
>> DC and fix this post-putback outside of VMC.
>>
>> Thanks,
>>
>> Team VMC (Joe and Glenn)
>> _______________________________________________
>> 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
