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. 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. 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 >
