Hi Jan,
On Wed, 16 Sep 2009, Jan Damborsky wrote:
> Hey Alok,
>
> please see my comments below.
> I reviewed all files, only those commented are mentioned.
Thanks for doing the review.
> 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.
Will do.
> cmd/auto-install/config/Makefile
> --------------------------------
> is line 39 needed ?
Nope, deleted.
>
> 53 if [[ "${strval}" = "" ]] ; then
> 54 return 0
> 55 fi
> 56 57 intval=$strval
> 58 return 0
>
> ->
>
> 53 [[ -n $strval ]] && intval=$strval
> 54 return 0
Changed.
> 65 - why default values are not viable ?
No good reason other than the fact that other parts
of the code were using the same defaults. Removed.
> 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.
It turns out that wget sends all output to stderr so
this is a no-op. Removed.
> 81-85 - code should be one tab indented
Done.
> cmd/auto-install/svc/auto-installer
> -----------------------------------
> 75 - I think we could take advantage of devprop(1M) instead
> of parsing prtconf(1M) output.
I actually can't get devprop(1M) to give me any output
on sparc or x86, I'll play with it some more since it
seems much cleaner.
> 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 ?
The 'shutdown' feature was implemented like this for the
VMC project. It turns out that if 'shutdown' is implemented
as part of the AI manifest instead, the VMC project would have to
modify the manifest contained within the bootable ai image.
And, in doing that they would have to pick apart solaris.zlib
and reassemble it.
This wasn't acceptable to that project and that's why specifying
it on the grub line was implemented.
> 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' ?
We do also want the users to specify 'boot net:dhcp - install'
to explicitly confirm the installation. And, we want something
similar for x86 as well. This is what we've been referring to as
the "accidental reboot" issue in AI.
Fixing the "accidental reboot" in AI wasn't a requirement for
the Bootable AI project however and that is why you don't see
any changes to address that issue here.
> 56-108 this section should be moved after do_service_discovery()
> function definition
Done
> 57,59,138,139,142,143,146,147,157,163,179,189,202,207,213,225
> '\' are redundant and can be removed.
Removed.
> 70-71
> I assume that this conditions determines if wanboot took place.
> If this is the case, please add comment explaining this mechanism.
Done.
> 110 - please add comment section for the function
Done.
> 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.
Will change.
> 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 ?
Renamed to ai_plat_setup
> 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.
That's 'hg merge' doing the auto-merge incorrectly.
Added it back.
> 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.
I would need to check what documentation references this
file before deleting it. I've also added this change to
all_lang_slim_cd_x86.xml.
> 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.
Thanks for pointing this out! I was not aware of this until
you pointed it out.
I will play around with devprop(1M) and if it doesn't work
as expected I'll use prtconf for now. The getbootargs utility
will be removed.
> 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.
I like that better than live-fs-root, will change.
> 33 - why comment is being removed if the code itself is not ?
Restored.
> 128-130, 338-340 - those line are not needed and could be removed
Done.
> lib/libtransfer/transfer_mod.py
> -------------------------------
>
> why is timeout feature implemented for 'pkg image-create', but
> not for other pkg operations, e.g. 'pkg install' ?
The problem that the timeout feature was intending to solve
was one of network configuration not being complete before
the 'pkg image-create' call. And, thus, initial contact with
the IPS repo would fail as well.
I can see the argument for extending the timeout for all
the other 'pkg' operations as well, it would probably be
useful in cases where there are intermittent connectivity
failures.
I'm ambivalent about making this change at this point but
if you or others feel strongly, I could make the change.
Thanks again for the review,
Alok
