Hey Alok,
please see my comments below.
I reviewed all files, only those commented are mentioned.
Thank you,
Jan
common comment
--------------
please make sure Copyright date is correct -
there are files (like auto-install/config/Makefile) with old
date. 'hg pbchk' will capture this.
cmd/auto-install/config/Makefile
--------------------------------
is line 39 needed ?
53 if [[ "${strval}" = "" ]] ; then
54 return 0
55 fi
56
57 intval=$strval
58 return 0
->
53 [[ -n $strval ]] && intval=$strval
54 return 0
65 - why default values are not viable ?
66 - do we want to discard the output of wget ?
It provides useful progress report. As I don't know
sure we want to show it in this case, thus checking.
81-85 - code should be one tab indented
cmd/auto-install/svc/auto-installer
-----------------------------------
75 - I think we could take advantage of devprop(1M) instead
of parsing prtconf(1M) output.
Speaking about implementation of 'shutdown' feature, why was different
approach taken comparing to 'auto-reboot' which is provided via AI manifest ?
I think it might be confusing and it seems that is works only for x86 -
why is it envisioned to work in Sparc case ?
cmd/auto-install/svc/manifest-locator
-------------------------------------
62 - we enforce to provide 'install' boot parameter if installation
is to be performed from media. If not specified, installation process
is not kicked off.
How is it going to work in 'net boot' case ? Don't we also want user
to explicitly confirm the installation by specifying
'boot net:dhcp - install' ?
56-108 this section should be moved after do_service_discovery()
function definition
57,59,138,139,142,143,146,147,157,163,179,189,202,207,213,225
'\' are redundant and can be removed.
70-71
I assume that this conditions determines if wanboot took place.
If this is the case, please add comment explaining this mechanism.
110 - please add comment section for the function
159,165,...
Since this code is now part of function, I think the correct approach
would be to return with failure to caller which would then exit with
$SMF_EXIT_ERR_FATAL.
cmd/distro_const/auto_install/Makefile
--------------------------------------
it seems that the convention is to list Python finalizer scripts w/o .py
suffix, shouldn't be done the same for ai_plat_setup.py ?
cmd/distro_const/auto_install/ai_sparc_image.xml,
cmd/distro_const/auto_install/ai_x86_image.xml
------------------------------------------------
I can see SUNWmkcd is being removed - just checking if it is really
not needed, since it was added by fix for bug 10486.
cmd/distro_const/slim_cd/slim_cd_x86.xml
----------------------------------------
Why are changes being done in this file ? I am asking, since I think
it is no longer used, as LiveCD with reduced set of languages is not
officially generated, maybe we could delete it in order to avoid
confusion.
It seems that these changes should go into all_lang_slim_cd_x86.xml instead.
cmd/install-tools/Makefile,
cmd/install-tools/getbootargs.c
-------------------------------
Is it necessary to write new utility to obtain boot parameters ?
It seems they are provided in device tree and we already have
mechanism for obtaining those values worked out:
# prtconf -vp | grep bootargs
bootargs: '- install prompt'
I would assume that devprop(1M) should be able to obtain that
value, but it for some reason does not work on Sparc -
it looks like it might be devprop(1M) bug.
cmd/slim-install/svc/live-fs-root
---------------------------------
I think that name of the method as well as service instance is now
a little bit misleading, since it takes care of both AI & LiveCD
media. Since we are introducing net-fs-root for boot from net,
I am wondering if it might be better to rename this method to
something like media-fs-root.
33 - why comment is being removed if the code itself is not ?
128-130, 338-340 - those line are not needed and could be removed
lib/libtransfer/transfer_mod.py
-------------------------------
why is timeout feature implemented for 'pkg image-create', but
not for other pkg operations, e.g. 'pkg install' ?
Alok Aggarwal wrote:
>
> On Fri, 11 Sep 2009, Alok Aggarwal wrote:
>
>> This is a code review request for the Bootable AI project [1].
>
> [ .. ]
>
>> e) Changes to auto-install and the transfer module to have a
>> built in timeout for the initial 'pkg' contact
>>
>> The last one (e) shows up in the webrev but still needs more
>> work and testing. I will send a follow on webrev some time next
>> week that has (e) in a more finished state.
>>
>> The webrev location is -
>>
>> http://cr.opensolaris.org/~aalok/bootable.ai/
>
> I have updated the webrev with finished changes for (e), the
> last remaining item.
>
> Please provide your review comments by Thursday, 9/17, 12pm PT.
>
> Alok
> _______________________________________________
> caiman-discuss mailing list
> caiman-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
