Hi Alok,
> 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.
>
Ok, I must have missed this.
>> ****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
> ly
>>
>> 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.
>
Why does it have to be currently coded for AI? I mean, if AI is the only
net consumer, why do we have to check?
>> 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,
sarah
******
>
> Thanks,
> Alok
