Joseph J. VLcek wrote:
> Thanks for the feedback Sarah. My comments below.
>
>
>
> Sarah Jelinek wrote:
>> Hi Glenn,
>>
>> Apologies for the delay in getting back to you on this. A couple of nits:
>> 1. The comment in the webrev should be the bug id, synopsis before
>> putback
>>
>> 2. vmc_common:
>> -Why are the vmc_error_handler_trap() and vmc_error_handler() not
>> prefixed with the 'function' qualifier? They are used in other
>> scripts, right? Maybe I misunderstand the use of 'function' in ksh93.
> I commented this issue in the function headers:
>
> 150 # This function is not defined using the function keyword
> 151 # to avoid an exit loop.
>
> Did you miss that comment or is more description needed?
Hi Joe,
It isn't the description it's that we don't have this method defined as
a 'function'. Which may be fine, I am just wondering what the difference is.
For example, in this file you have:
> 131 function vmc_set_new_iso_path
> 132 {
But, for this method there is no 'function' qualifier in the declaration:
> 164 vmc_error_handler_trap ()
> 165 {
Just wondering what the difference is between these methods.
Also, I just noticed the comment block above the vmc_set_new_iso_path is
incorrect:
> 118 #######################################################################
> 119 #
> 120 # vmc_is_vbox_installed
> 121 #
Should be:
# vmc_set_new_iso_path.
thanks,
sarah
***
>
> Joe
>
>>
>>
>> that's all I have.
>>
>> thanks,
>> sarah
>> ****
>> 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.which
>>> 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
>
