Hi Sarah,
On Sat, 19 Sep 2009, Sarah Jelinek wrote:
> 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 :-).
Thanks for the review.
> ****svc/auto-install
> line 75: /usr/sbin/prtconf should be defined above: i.e.
> PRTCONF=/usr/sbin/prtconf
Done.
> ****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).
Done.
> ****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?
This service is actually common across all our installers.
A suggestion during code review was to call the instance
'default' and I've made it so.
> ****slim_cd_x86.xml:
> Why don't we set compression for usr zlib to lzma in this case?
I'm not sure if we want to change the compression type for the
limited set of languages, the contents of that cd have tended
to be under the CD size.
> 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?
It isn't, I still need to update the defects with the suggested
changes.
> ****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
This is how it is done for Nevada so this is what I did here
as well :)
I do think checking the return value from the dd is a good
idea, will add that.
> ****auto_install/ai_sparc_image.xml
> line 356: doesn't require the indent.
Will change.
> 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?
Will do.
> ****getbootargs.c:
>> op = (struct openpromio *)malloc(OBP_MAXPATHLEN);
>
> Should probably check that op is not NULL.
I've actually deleted this utility entirely in favor of using
prtconf to get the same information.
> ****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.
Will remove.
> ****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?
The AI media is constructed with root:live-media enabled by
default and root:net disabled by default.
Here the check is made, for each architecture, on whether
we're booted off of the network. And if we are, we enable
the root:net instance and disable the root:live-media instance
(or actually root:media now after the code review comments).
> 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?
We do fail up above if with an SMF_EXIT_ERR_FATAL if we can't
find the boot device or the media.
It's only after we've processed the whole method and haven't
run into any errors that we return an SMF_EXIT_OK.
> ****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
Will change.
> 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?
Currently, it is intended only for AI. As we add more installers,
we'd have to evolve this method as well.
> Can we remove:
> lines 294-358?
Will do.
> line 352: can you put the keyboard setting with the svccfg apply nwam in the
> same conditional?
Done.
> ****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?
I've extracted out the linker cache stuff into the smf include
file. I'll also extract out the mounting of the optimal libc_psr.
Thanks,
Alok
