Hi Alok,
My comments.. I reviewed all files, only those noted have comments. I
tried not to repeat with others said, but if I have, feel free to just
set me straight on this, and I will re-read the thread :-).
****svc/auto-install
line 75: /usr/sbin/prtconf should be defined above: i.e.
PRTCONF=/usr/sbin/prtconf
****ai_plat.py:
> if (len(sys.argv) != 6): # Don't forget sys.argv[0] is the script itself.
> 62 raise Exception, (sys.argv[0] + ": Requires 5 args:\n" +
> 63 " Reader socket, pkg_image area, temp dir,\n" +
> 64 " bootroot build area, media area, grub setup type.")
Text should say "Requires 6 args:..."(not 5).
****ai_generic_live.xml:
> <service name='system/filesystem/root-minimal' version='1' type='service'>
> 163 <instance name='live-media' enabled='true'/>
> 164 </service>
Why is the instance name 'live-media' and not ai-media or something like
that?
****slim_cd_x86.xml:
Why don't we set compression for usr zlib to lzma in this case?
I know you haven't gotten to this yet, but bug information, which
doesn't seem complete, in the webrev would be good. For example, the
auto_install/ai_setup.py finalizer script isn't a fix for either of the
2 bugs you list is it?
****create_iso:
> /usr/bin/dd if="${BR_BUILD}/platform/`uname -m`/lib/fs/hsfs/bootblk" \
> 106 of="${PKG_IMG_PATH}/boot/hsfs.bootblock" \
> 107 bs=1b oseek=1 count=15 conv=sync 2> /dev/null
We don't check if dd succeeded before calling mkisofs. Should we? Also,
is output valuable or too much data? Wondering why we pipe it to /dev/null
****auto_install/ai_sparc_image.xml
line 356: doesn't require the indent.
The grub_setup.py script seems like, at least in part, redundant
functionality to what we provide in ict, and also on the AI server side.
Can you file a bug to combine these into one grub setup class?
****getbootargs.c:
> op = (struct openpromio *)malloc(OBP_MAXPATHLEN);
Should probably check that op is not NULL.
****live-fs-root:
starting at line 158
Can we get rid of this code? WE don't use cachefs or NFS as the backing
filesystem any more. Same in net-fs-root.
****live-fs-root-minimal:
> 7 if [ $ISA_INFO = "sparc" ]; then
> 78 # check if the wanboot device exists
> 79 BOOTFS_DISK="/devices/ramdisk-bootfs:a"
> 80 if [ -b "$BOOTFS_DISK" ]; then
> 81 #
> 82 # booting off of the net
> 83 /usr/sbin/svcadm enable svc:/system/filesystem/root:net
> 84 /usr/sbin/svcadm disable
> svc:/system/filesystem/root:live-media
> 85 fi
> 86 else
> 87 MEDIA=`$PRTCONF -v | $SED -n '/install_media/{;n;p;}'`
> 88 if [ ! -z "$MEDIA" ]; then
> 89 /usr/sbin/svcadm enable svc:/system/filesystem/root:net
> 90 /usr/sbin/svcadm disable
> svc:/system/filesystem/root:live-media
> 91 fi
> 92 fi
Why in both cases do you enable root:net, and disable root:live-media?
Also, why do we always return SMF_EXIT_OK for this script? If we don't
find the boot device or media then we should fail, right?
****net-fs-root:
line 165: > /usr/lib/fs/tmpfs/mount swap /tmp
This should use the MOUNT constant, and also pipe messages to /dev/msglog
Also, consider using a constant for /usr/bin/wget
It looks like this script is only coded for AI over the net. Can't it be
used for other net images later down the line? Say, text installer over
the net? I am wondering if we should exit if we don't find /.autoinstall?
Can we remove:
lines 294-358?
line 352: can you put the keyboard setting with the svccfg apply nwam in
the same conditional?
****General note:
Seems like both net-fs-root and live-fs-root have a lot of similar code,
specifically:
# Update kernel driver.conf cache with any additional driver.conf
# files found on /usr, and device permissions from /etc/minor_perm.
#
/usr/sbin/devfsadm -I -P
[ -f /etc/.dynamic_routing ] && /usr/bin/rm -f /etc/.dynamic_routing
libc_mount
#
# Discover architecture and find and mount optimal libc_psr
#
...
# Update runtime linker cache
Can we combine those into one script and include them? Or find some way
to only have 1 set of this functionality for both live-fs-root and
net-fs-root?
thanks,
sarah
****
Alok Aggarwal wrote:
>
> On Fri, 11 Sep 2009, Alok Aggarwal wrote:
>
>> http://cr.opensolaris.org/~aalok/bootable.ai/
>>
>> I would like to solicit feedback on this wad by Thursday, 12pm PT.
>> If you intend to review this code (or portions of it), please let
>> me know so I can plan accordingly.
>
> Forgot to add the date, due by Thursday, September 17,
> 12pm Pacific.
>
> Alok
> _______________________________________________
> caiman-discuss mailing list
> caiman-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
