Hi Jack,
Jack Schwartz wrote: > Hi Jan. > > Please treat my review as having an extra set of eyes on the code, not a > substitute for an expert in this area. Thank you for taking a look - all comments are appreciated ! > That said, here are my questions: > > Just curious: usr/src/lib/liborchestrator/Makefile: line 108: > > This Makefile is in a directory which looks platform generic, yet I see > a change to .../proto/root_i386/usr/snadm/lib. I see the same on line > 115, which was there before... (I assume this is the new installer as > this is a bugzilla bug not bugster bug.) Makes sense in that the new > installer runs only on X86... > > So how come this file isn't in a more platform-specific directory? Thank you for pointing this out. You are right that this code relates to the new installer, which for time of being runs only on x86. But sparc platform is not out of scope - we will need to resurrect sparc at some point - according to the Draft PSARC slides Dave has sent out recently, it seems that it might be delivered with Indiana 2H 2008. According to this, liborchestrator will address both platforms and thus using 'i386' specific path should be avoided - I will take advantage of macro $(ROOTADMINLIB) which will do the right thing in this case. > > - - - > > om_is_upgrade_target_valid() is now a stub. While removing references > to it may be beyond the scope of this bug, is it appropriate to open > another bug (or is there one already open) to remove all references to > it from other places? om_is_upgrade_target_valid() is called from GUI function upgrade-screen.c:validate_upgrade_target() for validation of Solaris instance to be upgraded. In Dwarf Caiman project it took care of validation of Solaris instance installed on UFS for purposes of in-place upgrade. Since in-place upgrade is no longer supported, the appropriate code was removed from om_is_upgrade_target_valid(). As time of being, GUI installer supports only initial installation, thus you are right that any code related to upgrade feature is not in-use for now. But since it seems probable that support for upgrade feature might be reenabled in GUI installer (UFS->ZFS migration, Snap upgrade ?), then om_is_upgrade_target_valid() would implement new code for validation of Solaris instance (either UFS or ZFS) to be upgraded. If you agree, I would keep it for now and if we are not sure about upgrade support in GUI I might file bug in order to be aware of this. If we later decide that only initial installation will be supported from GUI installer, all upgrade related code might be then removed from orchestrator as well as from GUI. Please let me know, what you think. Thank you, Jan > > Thanks, > Jack > > jan damborsky wrote: >> Hi Sundar, >> >> could I please ask you to review changes for >> following bug ? >> >> 683 orchestrator should be libspmi* clean >> http://defect.opensolaris.org/bz/show_bug.cgi?id=683 >> >> Webrev is available at >> http://cr.opensolaris.org/~dambi/bug-683/ >> >> If anybody else would like to take a look, >> it will be appreciated. >> >> Thank you very much, >> Jan >> >> _______________________________________________ >> caiman-discuss mailing list >> caiman-discuss at opensolaris.org >> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >> > > _______________________________________________ > caiman-discuss mailing list > caiman-discuss at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
