Thank you Ethan! Comments below.
As we discussed on the phone after I've addressed these issues I can push. Thanks again for the help! Joe Ethan Quach wrote: > > > Joseph J VLcek wrote: >> Thank you Ethan, >> >> See my comments in-line below. >> >> I've tested the updates using the manual set up with the ICT test >> exercisers as described below. >> >> A full SPARC AI test install is in the works. I will not push until >> this test succeeds. >> >> The updated webrev is available at: >> >> >> Ethan Quach wrote: >>> Joe, >>> >>> >>> ict.c - nit - 743, the string in this line now fits with the string in >>> the previous line so it could be moved up. >> >> done >> >>> ict.c - return_status could be set at line 883, but then clobbered >>> at 900. >> >> This is correct the way it is because return_status is set to the same >> error code on both line 883 and 900. > > Okay. > >> >>> ict.c - 886-903 - Instead of copying these files here, it would >>> seem to make more sense to augment ls_transfer() to take an array >>> of filenames to transfer along with the one known log file it >>> already copies. >> >> >> A summary of what we discussed on the phone: >> >> ls_transfer() should only transfer files created by logging service. >> Updating the logging service is outside of the scope of this bug. >> >> To track this I've filed enhancement bug: 7715 logging service should >> be more generic > > Thank you for filing this. > > > I just have a couple of nits... > > ict.py - 1618 - _get_root_dataset() doesn't get the root dataset > from the vfstab anymore so this error message is misleading. Can > you just drop the last two words from the error message? This is > also at 941. Done > > ict.c - 886-888 - Correlating IPS transfer mode to AI isn't > necessarily the right thing to assume. We may/will use the IPS > transfer mode with other installers in the future (e.g. the gui > installer), and these ai log files most likely won't exist in that > case. > > We don't currently have any official mechanism to determine if > we're doing an "AI install" per se, but that's really what we'd > need to base this code condition on. > > Since AI is the only thing that uses IPS transfer mode at this > time, this assumption works, but could you file a bug to address > this for the general case. I've filed: Bug 7720 - Need mechanism to determine if we're doing an "AI install" > > > thanks, > -ethan > >> >>> >>> ict.c - Would you also need a call to ict_log_print() for failure >>> and success before lines 942 and 944 respectively as well? >> >> done >> >>> >>> >>> ict.py - 144 - For Sparc, its not GRUB, so can you rename this >>> to ICT_CREATE_SPARC_BOOT_MENU_FAILED instead. >>> >>> ict.py - 347,348 - Same comment as above, replace GRUB in the >>> variable name with BOOT >>> >>> ict.py - 1596 - same comment, change function name to >>> create_sparc_boot_menu() >>> >>> ict.py - 1597-1685 - I think you get the idea; anywhere that >>> uses the word grub/GRUB in the context of Sparc, rework it to >>> use boot/BOOT. >> >> I think I got them all. >> % grep -i sparc usr/src/lib/libict_pymod/ict.py | grep -ic grub >> 0 >> >> >> I also updated install-finish to use the new name. >> >>> >>> ict.py - 1617 - Don't hardcode in this dataset path. >>> Looks like theres a function called _get_root_dataset() >>> which is what you'd want to use. >> >> done >> >>> >>> >>> thanks, >>> -ethan >>> >>> >>> >>> >>> Joseph J VLcek wrote: >>>> Could I please get a code review for the fixes for bugs 6744, 4407 & >>>> 7570. >>>> >>>> Bugs: >>>> ----- >>>> >>>> 6744 beadm fails for sparc AI sun4v due to missing /rpool/boot/menu.lst >>>> http://defect.opensolaris.org/bz/show_bug.cgi?id=6744 >>>> >>>> 4407 AI should also tranfer the ai_sd_log and the revelant xml to >>>> the system after install >>>> http://defect.opensolaris.org/bz/show_bug.cgi?id=4407 >>>> >>>> 7570 boot -L does not work on sparc due to missing >>>> <rootpool>/platform/`uname -m`/bootlst >>>> http://defect.opensolaris.org/bz/show_bug.cgi?id=7570 >>>> >>>> Webrev: >>>> ------- >>>> http://cr.opensolaris.org/~joev/bug6744_4407_7570 >>>> >>>> Brief description: >>>> ------------------ >>>> >>>> The majority of the changes involve: >>>> >>>> Two new ICT added to ict.py: >>>> - create_sparc_grub_menu (fix for: bug6744) >>>> - copy_sparc_bootlst (fix for: bug7570) >>>> >>>> One updated ICT in ict.c >>>> - ict_transfer_logs (fix for bug 4407) >>>> >>>> I made some minor fixes to the ict_test.c test program. >>>> >>>> Some changes involve small cleanup and rearranging as needed for >>>> these changes. >>>> >>>> The modules affected and tested: >>>> -------------------------------- >>>> liborchestrator >>>> liblogsvc <- small change >>>> install-finish >>>> ICT, Library and Python >>>> >>>> Testing done on SPARC >>>> --------------------- >>>> >>>> 1. >>>> I manually set up an AI install environment on a SPARC system. I >>>> then used the ict_test program and the ict.py exerciser to invoke >>>> the relevant ICT. >>>> >>>> 2. >>>> I built a SPARC AI image to include my updated bits. Then a full AI >>>> install was performed. (Thank you Jan for the help with this.) >>>> >>>> >>>> 3. >>>> Testing done on x86 (to confirm no regressions:) >>>> >>>> - Booted LiveCD image in VirtualBox >>>> - using ld_preload to load updated libraries >>>> - mount -F lofs to mount updated ict.py >>>> - cp updated install-script to /sbin >>>> - performed install >>>> >>>> * Results: >>>> All ICT completed successfully and system booted >>>> >>>> >>>> Thank you! >>>> >>>> Joe >>>> >>>> _______________________________________________ >>>> caiman-discuss mailing list >>>> caiman-discuss at opensolaris.org >>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>
